Good improvement especially in coding style from which I learned difference between rc and ret. :) > -----Original Message----- > From: Laine Stump [mailto:laine@xxxxxxxxx] > Sent: Tuesday, May 13, 2014 7:52 PM > To: libvir-list@xxxxxxxxxx; 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@xxxxxxxxxx>". > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list