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
