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

On 1/6/21 3:40 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 | 225 +++++++++++++++++------------------------- >   src/util/virnetlink.h |   4 +- >   2 files changed, 94 insertions(+), 135 deletions(-) > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > index fdcb0dc0..2936a3ef 100644 > --- a/src/util/virnetlink.c > +++ b/src/util/virnetlink.c > @@ -353,6 +353,52 @@ 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) This needs to be *resp == NULL, because now we're passing a pointer to a pointer.

> +        goto malformed_resp; > + > +    if ((*resp)->nlmsg_type == NLMSG_ERROR) { > +        struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp); And this needs to be NLMSG_DATA(*resp);

Also, might be worth putting an empty line here to create two blocks: one with variable declaration and the other with the code.

> +        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; > +            else > +                virReportSystemError(-err->error, > +                                     "%s", _("netlink error")); Since this is a two line body, it should be wrapped in curly braces (according to our coding style) which means that both bodies should have them (we don't really like one body having them while the other not).

> + > +            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 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, >       return 0; >   } >   > + >   /** >    * virNetlinkDumpLink: >    * > @@ -420,15 +467,14 @@ 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; > +    int error = 0; >   >       if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) >           return -1; > @@ -459,46 +505,23 @@ 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, &error, NULL) < 0) { > +        virReportSystemError(error, > +                             _("error dumping %s (%d) interface"), > +                             ifname, ifindex); >           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); > -            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 +546,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 +613,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; > -        } > -        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; >       } This is not correct IMO. The way that control flows through this function before your patch is (I'm trying to create virbr0 using 'virsh net-start default'): 1) virNetlinkCommand() suceeds, 2) resp->nlmsg_type == NLMSG_ERROR even though the bridge was created, 3) err->error is 0 hence we hit 'break' and fall out of the switch and .. >   >       return 0;

.. return 0. With your patch, I get the "malrofmed netlink response message" error. Reading netlink(7) manpage lets more light into this:    For reliable transfer the sender can request an acknowledgement from    the receiver by setting the NLM_F_ACK flag. An acknowledgment is an    NLMSG_ERROR packet with the error field set to 0. Which is exactly what I'm seeing, but what I don't understand is why we are getting this ack message since NLM_F_ACK flag was not set.

Michal