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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-01-11 at 19:26, Michal Privoznik wrote:
>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@xxxxxxxxxxxxxx>
>> ---
>>   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
> 

I agree. Thanks!

Shi Lei




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux