[libvirt] [PATCH] util: refactor virNetlinkCommand to fix several bugs / style problems
Wangrui (K)
moon.wangrui at huawei.com
Tue May 13 13:18:48 UTC 2014
Good improvement especially in coding style from which I learned difference between rc and ret.
:)
> -----Original Message-----
> From: Laine Stump [mailto:laine at laine.org]
> Sent: Tuesday, May 13, 2014 7:52 PM
> To: libvir-list at redhat.com; Wangrui (K); Yanqiangjun; Zhangbo (Oscar)
> Subject: [PATCH] util: refactor virNetlinkCommand to fix several bugs / style
> problems
>
> Inspired by a simpler patch from "Wangrui (K) <moon.wangrui at huawei.com>".
>
> A submitted patch pointed out that virNetlinkCommand() was doing an
> improper typecast of the return value from nl_recv() (int to
> unsigned), causing it to miss error returns, and that even after
> remedying that problem, virNetlinkCommand() was calling VIR_FREE() on
> the pointer returned from nl_recv() (*resp) even if nl_recv() had
> returned an error, and that in this case the pointer was verifiably
> invalid, as it was pointing to memory that had been allocated by
> libnl, but then freed prior to returning the error.
>
> While reviewing this patch, I noticed several other problems with this
> seemingly simple function (at least one of them as serious as the
> problem being reported/fixed by the aforementioned patch), and decided
> they all deserved to be fixed. Here is the list:
>
> 1) The return value from nl_recv() must be assigned to an int (rather
> than unsigned int) in order to detect failure.
>
> 2) When nl_recv() returns an error, the contents of *resp is invalid,
> and should be simply set to 0, *not* VIR_FREE()'d.
>
> 3) The first error return from virNetlinkCommand returns -EINVAL,
> incorrectly implying that the caller can expect the return value to
> be of the "-errno" variety, which is not true in any other case.
>
> 4) The 2nd error return returns directly with garbage in *resp. While
> the caller should never use *resp in this case, it's still good
> practice to set it to NULL.
>
> 5) For the next 5 (!!) error conditions, *resp will contain garbage,
> and virNetlinkCommand() will goto it's cleanup code which will
> VIR_FREE(*resp), almost surely leading to a segfault.
>
> In addition to fixing these 5 problems, this patch also makes the
> following two changes to make the function conform more closely to the
> style of other libvirt code:
>
> 1) Change the handling of return code from "named rc and defaulted to
> 0, but changed to -1 on error" to the more common "named ret and
> defaulted to -1, but changed to 0 on success".
>
> 2) Rename the "error" label to "cleanup", since the code that follows
> is executed in success cases as well as failure.
> ---
> src/util/virnetlink.c | 42 ++++++++++++++++++++----------------------
> 1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index 0cf18f2..db8d59e 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2010-2013 Red Hat, Inc.
> + * Copyright (C) 2010-2014 Red Hat, Inc.
> * Copyright (C) 2010-2012 IBM Corporation
> *
> * This library is free software; you can redistribute it and/or
> @@ -180,7 +180,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
> uint32_t src_pid, uint32_t dst_pid,
> unsigned int protocol, unsigned int groups)
> {
> - int rc = 0;
> + int ret = -1;
> struct sockaddr_nl nladdr = {
> .nl_family = AF_NETLINK,
> .nl_pid = dst_pid,
> @@ -192,41 +192,39 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
> int n;
> struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg);
> virNetlinkHandle *nlhandle = NULL;
> + int len = 0;
>
> if (protocol >= MAX_LINKS) {
> virReportSystemError(EINVAL,
> _("invalid protocol argument: %d"),
> protocol);
> - return -EINVAL;
> + goto cleanup;
> }
>
> nlhandle = virNetlinkAlloc();
> if (!nlhandle) {
> virReportSystemError(errno,
> "%s", _("cannot allocate nlhandle for
> netlink"));
> - return -1;
> + goto cleanup;
> }
>
> if (nl_connect(nlhandle, protocol) < 0) {
> virReportSystemError(errno,
> _("cannot connect to netlink socket with
> protocol %d"),
> protocol);
> - rc = -1;
> - goto error;
> + goto cleanup;
> }
>
> fd = nl_socket_get_fd(nlhandle);
> if (fd < 0) {
> virReportSystemError(errno,
> "%s", _("cannot get netlink socket fd"));
> - rc = -1;
> - goto error;
> + goto cleanup;
> }
>
> if (groups && nl_socket_add_membership(nlhandle, groups) < 0) {
> virReportSystemError(errno,
> "%s", _("cannot add netlink
> membership"));
> - rc = -1;
> - goto error;
> + goto cleanup;
> }
>
> nlmsg_set_dst(nl_msg, &nladdr);
> @@ -237,8 +235,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
> if (nbytes < 0) {
> virReportSystemError(errno,
> "%s", _("cannot send to netlink socket"));
> - rc = -1;
> - goto error;
> + goto cleanup;
> }
>
> memset(fds, 0, sizeof(fds));
> @@ -253,26 +250,27 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
> if (n == 0)
> virReportSystemError(ETIMEDOUT, "%s",
> _("no valid netlink response was
> received"));
> - rc = -1;
> - goto error;
> + goto cleanup;
> }
>
> - *respbuflen = nl_recv(nlhandle, &nladdr,
> - (unsigned char **)resp, NULL);
> - if (*respbuflen <= 0) {
> + len = nl_recv(nlhandle, &nladdr, (unsigned char **)resp, NULL);
> + if (len <= 0) {
> virReportSystemError(errno,
> "%s", _("nl_recv failed"));
> - rc = -1;
> + goto cleanup;
> }
> - error:
> - if (rc == -1) {
> - VIR_FREE(*resp);
> +
> + ret = 0;
> + cleanup:
> + if (ret < 0) {
> *resp = NULL;
> *respbuflen = 0;
> + } else {
> + *respbuflen = len;
> }
>
> virNetlinkFree(nlhandle);
> - return rc;
> + return ret;
> }
>
> static void
> --
> 1.9.0
More information about the libvir-list
mailing list