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

On 1/11/21 3: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 | 232 ++++++++++++++++++------------------------ >   src/util/virnetlink.h |   4 +- >   2 files changed, 101 insertions(+), 135 deletions(-) > > diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c > index fdcb0dc0..650acff7 100644 > --- a/src/util/virnetlink.c > +++ b/src/util/virnetlink.c > @@ -353,6 +353,54 @@ int virNetlinkCommand(struct nl_msg *nl_msg, >       return 0; >   } >   > + I guess we should document that NLMSG_ERROR and err->error == 0 is a valid case and no error. How about: /**   * virNetlinkTalk:   * @ifname: name of the link   * @nl_msg: pointer to netlink message   * @src_pid: pid used for nl_pid of the local end of the netlink message   *           (0 == "use getpid()")   * @dst_pid: pid of destination nl_pid if the kernel   *           is not the target of the netlink message but it is to be   *           sent to another process (0 if sending to the kernel)   * @resp: pointer to pointer where response buffer will be allocated   * @resp_len: pointer to integer holding the size of the response buffer   *            on return of the function   * @error: pointer to store netlink error (-errno)   * @fallback: pointer to an alternate function that will be called in case   *            netlink fails with EOPNOTSUPP (any other error will simply be   *            treated as an error)   *   * Simple wrapper around virNetlinkCommand(). The returned netlink message   * is allocated at @resp. Please note that according to netlink(7) man page,   * reply with type of NLMSG_ERROR and @error == 0 is an acknowledgment and   * thus not an error.   *   * Returns: 0 on success,   *         -1 otherwise (error reported if @error == NULL)   */ > +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; > + > +        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; So this sets @error to a negative number. [1] > +            else > +                virReportSystemError(-err->error, "%s", _("netlink error")); > + > +            return -1; > +        } And here I'd put:          /* According to netlink(7) man page NLMSG_ERROR packet with error           * field set to 0 is an acknowledgment packet and thus not an error. */ > +    } > + > +    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 +444,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, >       return 0; >   } >   > + >   /** >    * virNetlinkDumpLink: >    * > @@ -420,15 +469,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 +507,25 @@ 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, 1: and here it is used as if it was positive. We need -error. And in the rest of places too. Anyway, I can fix that before pushing, so that you don't have to send another version. Do you agree if this is squashed in? diff --git c/src/util/virnetlink.c w/src/util/virnetlink.c index 650acff7d7..9bd7339054 100644 --- c/src/util/virnetlink.c +++ w/src/util/virnetlink.c @@ -354,6 +354,31 @@ int virNetlinkCommand(struct nl_msg *nl_msg,   } +/** + * virNetlinkTalk: + * @ifname: name of the link + * @nl_msg: pointer to netlink message + * @src_pid: pid used for nl_pid of the local end of the netlink message + *           (0 == "use getpid()") + * @dst_pid: pid of destination nl_pid if the kernel + *           is not the target of the netlink message but it is to be + *           sent to another process (0 if sending to the kernel) + * @resp: pointer to pointer where response buffer will be allocated + * @resp_len: pointer to integer holding the size of the response buffer + *            on return of the function + * @error: pointer to store netlink error (-errno) + * @fallback: pointer to an alternate function that will be called in case + *            netlink fails with EOPNOTSUPP (any other error will simply be + *            treated as an error) + * + * Simple wrapper around virNetlinkCommand(). The returned netlink message + * is allocated at @resp. Please note that according to netlink(7) man page, + * reply with type of NLMSG_ERROR and @error == 0 is an acknowledgment and + * thus not an error. + * + * Returns: 0 on success, + *         -1 otherwise (error reported if @error == NULL) + */   static int   virNetlinkTalk(const char *ifname,                  virNetlinkMsg *nl_msg, @@ -390,6 +415,9 @@ virNetlinkTalk(const char *ifname,               return -1;           } + +        /* According to netlink(7) man page NLMSG_ERROR packet with error +         * field set to 0 is an acknowledgment packet and thus not an error. */       }       return 0; @@ -509,7 +537,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex,       if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid,                          &resp, &resp_len, &error, NULL) < 0) { -        virReportSystemError(error, +        virReportSystemError(-error,                                _("error dumping %s (%d) interface"),                                ifname, ifindex);           return -1; @@ -668,7 +696,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback)       if (virNetlinkTalk(ifname, nl_msg, 0, 0,                          &resp, &resp_len, &error, fallback) < 0) { -        virReportSystemError(error, +        virReportSystemError(-error,                                _("error destroying network device %s"),                                ifname);           return -1; @@ -720,7 +748,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)       if (virNetlinkTalk(NULL, nl_msg, src_pid, dst_pid,                          &resp, &resp_len, &error, NULL) < 0) { -        virReportSystemError(error, "%s", _("error dumping")); +        virReportSystemError(-error, "%s", _("error dumping neighbor table"));           return -1;       } Michal