[libvirt] [PATCHv2 2/4] Add wrapper macros around nla_nest*/nla_put* to make code more readable
Shi Lei
shi_lei at massclouds.com
Thu Sep 6 08:09:35 UTC 2018
On 2018-09-05 at 23:01, Erik Skultety wrote:
>On Wed, Sep 05, 2018 at 04:36:28PM +0800, Shi Lei wrote:
>> This patch adds wrapper macros around nla_nest*/nla_put* which apply to
>> virNetlinkNewLink and virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink.
>>
>> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
>> ---
>> src/util/virnetdev.c | 54 ++++++--------
>> src/util/virnetdevvportprofile.c | 117 ++++++++++---------------------
>> src/util/virnetlink.c | 74 +++++++------------
>> src/util/virnetlink.h | 52 ++++++++++++++
>> 4 files changed, 134 insertions(+), 163 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 8eac419..892a147 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1657,11 +1657,11 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>> {
>> int rc = -1;
>> char macstr[VIR_MAC_STRING_BUFLEN];
>> - struct nlmsghdr *resp = NULL;
>> struct nlmsgerr *err;
>> unsigned int recvbuflen = 0;
>> - struct nl_msg *nl_msg;
>> struct nlattr *vfinfolist, *vfinfo;
>> + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
>> + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
>
>changes related to AUTO{PTR,FREE} should completely separate, even from the
>series itself.
Okay.
>
>> struct ifinfomsg ifinfo = {
>> .ifi_family = AF_UNSPEC,
>> .ifi_index = -1,
>> @@ -1676,47 +1676,41 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>> return rc;
>> }
>>
>> - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>> - goto buffer_too_small;
>> -
>> - if (ifname &&
>> - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
>
>as I mentioned in my reply to the previous patch, I'd first replace ocurrences
>like ^these with nla_put_string for consistency and also making reviewer's life
>easier.
Okay.
Since the changes of virNetDevSetVfConfig and virNetDevVPortProfileOpSetLink
are unrelated with this series, I'll take them out and post them as another series.
>
>> - goto buffer_too_small;
>> -
>> + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error"));
>> + return rc;
>> + }
>>
>> - if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST)))
>> - goto buffer_too_small;
>> + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
>>
>> - if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
>> - goto buffer_too_small;
>> + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST);
>> + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
>>
>> if (macaddr) {
>> struct ifla_vf_mac ifla_vf_mac = {
>> - .vf = vf,
>> - .mac = { 0, },
>> + .vf = vf,
>> + .mac = { 0, },
>> };
>>
>> virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
>>
>> - if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac),
>> - &ifla_vf_mac) < 0)
>> - goto buffer_too_small;
>> + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC,
>> + sizeof(ifla_vf_mac), &ifla_vf_mac);
>> }
>>
>> if (vlanid >= 0) {
>> struct ifla_vf_vlan ifla_vf_vlan = {
>> - .vf = vf,
>> - .vlan = vlanid,
>> - .qos = 0,
>> + .vf = vf,
>> + .vlan = vlanid,
>> + .qos = 0,
>> };
>>
>> - if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan),
>> - &ifla_vf_vlan) < 0)
>> - goto buffer_too_small;
>> + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN,
>> + sizeof(ifla_vf_vlan), &ifla_vf_vlan);
>> }
>>
>> - nla_nest_end(nl_msg, vfinfo);
>> - nla_nest_end(nl_msg, vfinfolist);
>> + NETLINK_MSG_NEST_END(nl_msg, vfinfo);
>> + NETLINK_MSG_NEST_END(nl_msg, vfinfolist);
>>
>> if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
>> NETLINK_ROUTE, 0) < 0)
>> @@ -1767,20 +1761,12 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>> ifname, vf,
>> macaddr ? virMacAddrFormat(macaddr, macstr) : "(unchanged)",
>> vlanid, rc < 0 ? "Fail" : "Success");
>> -
>> - nlmsg_free(nl_msg);
>> - VIR_FREE(resp);
>> return rc;
>>
>> malformed_resp:
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("malformed netlink response message"));
>> goto cleanup;
>> -
>> - buffer_too_small:
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("allocated netlink buffer is too small"));
>> - goto cleanup;
>> }
>>
>> static int
>> diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
>> index 3ebf757..8574571 100644
>> --- a/src/util/virnetdevvportprofile.c
>> +++ b/src/util/virnetdevvportprofile.c
>> @@ -641,8 +641,6 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
>> int32_t vf,
>> uint8_t op)
>> {
>> - int rc = -1;
>> - struct nlmsghdr *resp = NULL;
>> struct nlmsgerr *err;
>> struct ifinfomsg ifinfo = {
>> .ifi_family = AF_UNSPEC,
>> @@ -651,12 +649,14 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
>> unsigned int recvbuflen = 0;
>> int src_pid = 0;
>> uint32_t dst_pid = 0;
>> - struct nl_msg *nl_msg;
>> struct nlattr *vfports = NULL, *vfport;
>> char macStr[VIR_MAC_STRING_BUFLEN];
>> char hostUUIDStr[VIR_UUID_STRING_BUFLEN];
>> char instanceUUIDStr[VIR_UUID_STRING_BUFLEN];
>> const char *opName;
>> + int vfport_type = IFLA_PORT_SELF;
>> + VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
>> + VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
>
>again the VIR_AUTO{PTR,FREE} stuff...
Okay.
>
>>
>> switch (op) {
>> case PORT_REQUEST_PREASSOCIATE:
>> @@ -691,24 +691,21 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
>> nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST);
>> if (!nl_msg) {
>> virReportOOMError();
>> - return rc;
>> + return -1;
>> }
>>
>> - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
>> - goto buffer_too_small;
>> + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nlmsg_append error"));
>> + return -1;
>> + }
>>
>> - if (ifname &&
>> - nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
>> - goto buffer_too_small;
>> + NETLINK_MSG_PUT_STRING(nl_msg, IFLA_IFNAME, ifname);
>>
>> if (macaddr || vlanid >= 0) {
>> struct nlattr *vfinfolist, *vfinfo;
>>
>> - if (!(vfinfolist = nla_nest_start(nl_msg, IFLA_VFINFO_LIST)))
>> - goto buffer_too_small;
>> -
>> - if (!(vfinfo = nla_nest_start(nl_msg, IFLA_VF_INFO)))
>> - goto buffer_too_small;
>> + NETLINK_MSG_NEST_START(nl_msg, vfinfolist, IFLA_VFINFO_LIST);
>> + NETLINK_MSG_NEST_START(nl_msg, vfinfo, IFLA_VF_INFO);
>>
>> if (macaddr) {
>> struct ifla_vf_mac ifla_vf_mac = {
>> @@ -718,9 +715,8 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
>>
>> virMacAddrGetRaw(macaddr, ifla_vf_mac.mac);
>>
>> - if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac),
>> - &ifla_vf_mac) < 0)
>> - goto buffer_too_small;
>> + NETLINK_MSG_PUT(nl_msg, IFLA_VF_MAC,
>> + sizeof(ifla_vf_mac), &ifla_vf_mac);
>> }
>>
>> if (vlanid >= 0) {
>> @@ -730,77 +726,49 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
>> .qos = 0,
>> };
>>
>> - if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan),
>> - &ifla_vf_vlan) < 0)
>> - goto buffer_too_small;
>> + NETLINK_MSG_PUT(nl_msg, IFLA_VF_VLAN,
>> + sizeof(ifla_vf_vlan), &ifla_vf_vlan);
>> }
>>
>> - nla_nest_end(nl_msg, vfinfo);
>> - nla_nest_end(nl_msg, vfinfolist);
>> - }
>> -
>> - if (vf == PORT_SELF_VF && nltarget_kernel) {
>> - if (!(vfport = nla_nest_start(nl_msg, IFLA_PORT_SELF)))
>> - goto buffer_too_small;
>> - } else {
>> - if (!(vfports = nla_nest_start(nl_msg, IFLA_VF_PORTS)))
>> - goto buffer_too_small;
>> -
>> - /* begin nesting vfports */
>> - if (!(vfport = nla_nest_start(nl_msg, IFLA_VF_PORT)))
>> - goto buffer_too_small;
>> - }
>
>at this point it got quite difficult to keep track of all the replacements, so
>I'd suggest making the replacements one at a time, first one macro, then
>another, etc.
Sorry for the trouble.
As I say above, I'll take those unrelated code out of this series,
except for virNetlinkNewLink.
>
>...
>
>> /**
>> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
>> index 8163a81..2e64570 100644
>> --- a/src/util/virnetlink.h
>> +++ b/src/util/virnetlink.h
>> @@ -48,6 +48,58 @@ struct nlmsghdr;
>>
>> # endif /* __linux__ */
>>
>> +
>> +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \
>> +do { \
>> + container = nla_nest_start(msg, attrtype); \
>> + if (!container) { \
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
>> + _("nla_nest_start error [" #attrtype "]")); \
>> + return -1; \
>> + } \
>> +} while(0)
>> +
>> +# define NETLINK_MSG_NEST_END(msg, container) \
>> +do { nla_nest_end(msg, container); } while(0)
>> +
>> +/*
>> + * When data is an rvalue, compiler will report error as below:
>> + * "the address of [...] will always evaluate as 'true' [-Werror=address]"
>> + * Add a dummy(as an lvalue) to solve it.
>
>Okay, let's try rewording this in a more compact manner below...
>
>> + */
>> +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \
>> +do { \
>
>how about:
>/* @data may not always be a pointer type, therefore making compilers complain
> * about the check below, so let's use an intermediary pointer type */
Okay. It's clear.
>
>> + const void *dummy = data; \
>
>let's rename @dummy to dataptr to make it more explicit
Okay.
>
>> + if (dummy && nla_put(msg, attrtype, datalen, data) < 0) { \
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \
>> + _("nla_put error [" #attrtype "]")); \
>> + return -1; \
>> + } \
>> +} while(0)
>> +
>> +# define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \
>> +do { \
>> + const void *dummy = str; \
>> + if (dummy && nla_put_string(msg, attrtype, str) < 0) { \
>> + virReportError(VIR_ERR_INTERNAL_ERROR, \
>> + _("nla_put_string error [" #attrtype "]: '%s'"), \
>> + str); \
>
>I don't think that changing the error is necessary, all of the variants
>essentially call nla_put and that one either fails with -EINVAL (not our case,
>since we always provide a valid datalen) or -ENOMEM, so the original error
>stating that the buffer wasn't large enough is sufficient.
Okay. Then I think that the code could be like below:
if (dummy && nla_put_string(msg, attrtype, str) < 0) { \
virReportError(VIR_ERR_NO_MEMORY, \
_("nla_put_string error [" #attrtype "]: '%s'"), \
str); \
return -ENOMEM; \
} \
But we should ensure that the return-value of those functions which call nla_put*
would accord with their declaration. I find that many of the functions
in util/virnetdev.c and util/virnetdev.c always return -1 when they fail, whatever the error.
For virNetlinkDumpLink/virNetlinkDelLink, their notes include stuff like below:
/* Returns 0 on success, -1 on fatal error. */
So I would do some work on these declaration...
>
>> + return -1; \
>> + } \
>> +} while(0)
>
>I'm not entirely sold on these MSG_PUT_X macros, but I don't have a reasonable
>argument against either, so let's leave them in. Alternatively, we could have
>
>#define NETLINK_MSG_PUT_STRING(msg, attrtype, str) \
> NETLINK_MSG_PUT(msg, attrtype, strlen(str) + 1, str)
>
>and that would make things more clean, afterall that is what all the
>nla_put_foo will do anyway...
>
>Erik
I understand. Most of the nla_put_foo actually use nla_put simply in current implementation.
But I have some other ideas on this point:
The authors of the libnl provide many nla_put_foo/nla_get_foo functions for various types
(e.g. s8/u8/s16/u16/.../string/msecs/...). Perhaps they are more than convenient helper-functions.
I suppose they might want users to use them as far as possible if the users know the attribute type
actually, because they would get more information about this attribute payload.
There's a chance that in future they could do some extra work in nla_put_foo/nla_get_foo
according to the exact type, such as type-check or optimization ...
So I think it's useful to provide these NETLINK_MSG_PUT_[type] which just wrap nla_put_[type].
Thanks for your patience. Have a nice day! :-)
Shi Lei
>
>> +
>> +# define NETLINK_MSG_PUT_U32PTR(msg, attrtype, ptr) \
>> +do { \
>> + const void *dummy = ptr; \
>> + if (dummy && nla_put_u32(msg, attrtype, *ptr) < 0) { \
>> + virReportError(VIR_ERR_INTERNAL_ERROR, \
>> + _("nla_put_u32 error [" #attrtype "]: '%u'"), \
>> + *ptr); \
>> + return -1; \
>> + } \
>> +} while(0)
>> +
>> +
>> int virNetlinkStartup(void);
>> void virNetlinkShutdown(void);
>>
>> --
>> 2.17.1
>>
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list