Re: Re: [PATCH 4/4] netlink: Introduce a helper function to simplify netlink functions

On 12/21/20 4:23 AM, Shi Lei wrote: > Extract common code as helper function virNetlinkTalk, then simplify > the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor]. > > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com&gt; > --- >   src/util/virnetlink.c | 210 +++++++++++++++--------------------------- >   src/util/virnetlink.h |   4 +- >   2 files changed, 78 insertions(+), 136 deletions(-) Nice cleanup. Patches 1-3 look good.

> > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > index fdcb0dc0..7bea38c0 100644 > --- a/src/util/virnetlink.c > +++ b/src/util/virnetlink.c > @@ -353,6 +353,48 @@ int virNetlinkCommand(struct nl_msg *nl_msg, >       return 0; >   } >   > + > +static int > +virNetlinkTalk(const char *ifname, > +               virNetlinkMsg *nl_msg, > +               uint32_t src_pid, > +               uint32_t dst_pid, > +               struct nlmsghdr **resp, > +               unsigned int *resp_len, > +               int *error, > +               virNetlinkTalkFallback fallback) > +{ > +    if (virNetlinkCommand(nl_msg, resp, resp_len, > +                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) > +        return -1; > + > +    if (*resp_len < NLMSG_LENGTH(0) || resp == NULL) > +        goto malformed_resp; > + > +    if ((*resp)->nlmsg_type == NLMSG_ERROR) { > +        struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp); > +        if ((*resp)->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) > +            goto malformed_resp; > + > +        if (-err->error == EOPNOTSUPP && fallback) > +            return fallback(ifname); > + > +        if (err->error < 0) { > +            if (error) > +                *error = err->error; I wonder whether we should report an error here. I mean, if err->error is set (and not EOPNOTSUPP) then it's stored into *error, good. But -1 is returned ... > +            return -1; .. here and it's indistinguishable to the caller from -1 returned in 'malformed_resp'. Looking at the usage in next hunks, how about:    if (error)      *error = err->error;    else      virReportSystemError(-err->error, ...);    return -1;

> +        } > +    } > + > +    return 0; > + > + malformed_resp: > +    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                   _("malformed netlink response message")); > +    return -1; > +} > + > + >   int >   virNetlinkDumpCommand(struct nl_msg *nl_msg, >                         virNetlinkDumpCallback callback, > @@ -396,6 +438,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, >       return 0; >   } >   > + >   /** >    * virNetlinkDumpLink: >    * > @@ -420,15 +463,13 @@ virNetlinkDumpLink(const char *ifname, int ifindex, >                      void **nlData, struct nlattr **tb, >                      uint32_t src_pid, uint32_t dst_pid) >   { > -    int rc = -1; > -    struct nlmsgerr *err; >       struct ifinfomsg ifinfo = { >           .ifi_family = AF_UNSPEC, >           .ifi_index  = ifindex >       }; > -    unsigned int recvbuflen; >       g_autoptr(virNetlinkMsg) nl_msg = NULL; >       g_autofree struct nlmsghdr *resp = NULL; > +    unsigned int resp_len = 0; >   >       if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) >           return -1; > @@ -459,46 +500,19 @@ virNetlinkDumpLink(const char *ifname, int ifindex, >       } >   # endif >   > -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, > -                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) > +    if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid, > +                       &resp, &resp_len, NULL, NULL) < 0) >           return -1; >   > -    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) > -        goto malformed_resp; > - > -    switch (resp->nlmsg_type) { > -    case NLMSG_ERROR: > -        err = (struct nlmsgerr *)NLMSG_DATA(resp); > -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) > -            goto malformed_resp; > - > -        if (err->error) { > -            virReportSystemError(-err->error, > -                                 _("error dumping %s (%d) interface"), > -                                 ifname, ifindex); Here we'd report an error.

> -            return -1; > -        } > -        break; > - > -    case GENL_ID_CTRL: > -    case NLMSG_DONE: > -        rc = nlmsg_parse(resp, sizeof(struct ifinfomsg), > -                         tb, IFLA_MAX, NULL); > -        if (rc < 0) > -            goto malformed_resp; > -        break; > - > -    default: > -        goto malformed_resp; > +    if ((resp->nlmsg_type != GENL_ID_CTRL && resp->nlmsg_type != NLMSG_DONE) || > +        nlmsg_parse(resp, sizeof(struct ifinfomsg), tb, IFLA_MAX, NULL) < 0) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                       _("malformed netlink response message")); > +        return -1; >       } >   >       *nlData = g_steal_pointer(&resp); >       return 0; > - > - malformed_resp: > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("malformed netlink response message")); > -    return rc; >   } >   >   > @@ -523,13 +537,12 @@ virNetlinkNewLink(const char *ifname, >                     virNetlinkNewLinkDataPtr extra_args, >                     int *error) >   { > -    struct nlmsgerr *err; >       struct nlattr *linkinfo = NULL; >       struct nlattr *infodata = NULL; > -    unsigned int buflen; >       struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; >       g_autoptr(virNetlinkMsg) nl_msg = NULL; >       g_autofree struct nlmsghdr *resp = NULL; > +    unsigned int resp_len = 0; >   >       *error = 0; >   > @@ -591,37 +604,17 @@ virNetlinkNewLink(const char *ifname, >           } >       } >   > -    if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0) > +    if (virNetlinkTalk(ifname, nl_msg, 0, 0, > +                       &resp, &resp_len, error, NULL) < 0) >           return -1; >   > -    if (buflen < NLMSG_LENGTH(0) || resp == NULL) > -        goto malformed_resp; > - > -    switch (resp->nlmsg_type) { > -    case NLMSG_ERROR: > -        err = (struct nlmsgerr *)NLMSG_DATA(resp); > -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) > -            goto malformed_resp; > - > -        if (err->error < 0) { > -            *error = err->error; > -            return -1; But here we wouldn't.

> -        } > -        break; > - > -    case NLMSG_DONE: > -        break; > - > -    default: > -        goto malformed_resp; > +    if (resp->nlmsg_type != NLMSG_DONE) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                       _("malformed netlink response message")); > +        return -1; >       } >   >       return 0; > - > - malformed_resp: > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("malformed netlink response message")); > -    return -1; >   } >   >   > @@ -641,13 +634,12 @@ virNetlinkNewLink(const char *ifname, >    * Returns 0 on success, -1 on fatal error. >    */ >   int > -virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) > +virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback) >   { > -    struct nlmsgerr *err; >       struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; > -    unsigned int recvbuflen; >       g_autoptr(virNetlinkMsg) nl_msg = NULL; >       g_autofree struct nlmsghdr *resp = NULL; > +    unsigned int resp_len = 0; >   >       nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST); >       if (!nl_msg) { > @@ -659,44 +651,17 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) >   >       NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); >   > -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, > -                          NETLINK_ROUTE, 0) < 0) { > +    if (virNetlinkTalk(ifname, nl_msg, 0, 0, > +                       &resp, &resp_len, NULL, fallback) < 0) >           return -1; > -    } >   > -    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) > -        goto malformed_resp; > - > -    switch (resp->nlmsg_type) { > -    case NLMSG_ERROR: > -        err = (struct nlmsgerr *)NLMSG_DATA(resp); > -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) > -            goto malformed_resp; > - > -        if (-err->error == EOPNOTSUPP && fallback) > -            return fallback(ifname); > - > -        if (err->error) { > -            virReportSystemError(-err->error, > -                                 _("error destroying network device %s"), > -                                 ifname); And here we would report an error again.

> -            return -1; > -        } > -        break; > - > -    case NLMSG_DONE: > -        break; > - > -    default: > -        goto malformed_resp; > +    if (resp->nlmsg_type != NLMSG_DONE) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                       _("malformed netlink response message")); > +        return -1; >       } >   >       return 0; > - > - malformed_resp: > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("malformed netlink response message")); > -    return -1; >   } >   >   /** > @@ -712,18 +677,17 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) >    * >    * Get neighbor table entry from netlink. >    * > - * Returns 0 on success, -1 on fatal error. > + * Returns length of the raw data from netlink on success, -1 on fatal error. >    */ >   int >   virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid) >   { > -    struct nlmsgerr *err; >       struct ndmsg ndinfo = { >           .ndm_family = AF_UNSPEC, >       }; > -    unsigned int recvbuflen; >       g_autoptr(virNetlinkMsg) nl_msg = NULL; >       g_autofree struct nlmsghdr *resp = NULL; > +    unsigned int resp_len = 0; >   >       nl_msg = nlmsg_alloc_simple(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST); >       if (!nl_msg) { > @@ -733,40 +697,18 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid) >   >       NETLINK_MSG_APPEND(nl_msg, sizeof(ndinfo), &ndinfo); >   > -    if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, > -                          src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) > +    if (virNetlinkTalk(NULL, nl_msg, src_pid, dst_pid, > +                       &resp, &resp_len, NULL, NULL) < 0) >           return -1; >   > -    if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) > -        goto malformed_resp; > - > -    switch (resp->nlmsg_type) { > -    case NLMSG_ERROR: > -        err = (struct nlmsgerr *)NLMSG_DATA(resp); > -        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) > -            goto malformed_resp; > - > -        if (err->error) { > -            virReportSystemError(-err->error, > -                                 "%s", _("error dumping")); > -            return -1; Ditto.

> -        } > -        break; > - > -    case RTM_NEWNEIGH: > -        break; > - > -    default: > -        goto malformed_resp; > +    if (resp->nlmsg_type != RTM_NEWNEIGH) { > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > +                       _("malformed netlink response message")); > +        return -1; >       } >   >       *nlData = g_steal_pointer(&resp); > -    return recvbuflen; > - > - malformed_resp: > -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > -                   _("malformed netlink response message")); > -    return -1; > +    return resp_len; >   } >   Michal