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@xxxxxxxxxxxxxx>
> ---
> 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.
> 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.
> - 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...
>
> 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.
...
> /**
> 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 */
> + const void *dummy = data; \
let's rename @dummy to dataptr to make it more explicit
> + 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.
> + 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
> +
> +# 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
