[libvirt] Re: libvirt code review

[Daniel Veillard]

On Thu, Nov 05, 2009 at 03:27:16PM -0500, Steve Grubb wrote:
> Hello,
> 
> I missed code in a couple other directories. This is much shorter.
> 
> -Steve
> 
> 
> In daemon/libvirtd.c at line 371 the "if" statement will never be true. ret is
> set to 0 at line 350 and never changed.

  probably a leftover from previous refactoring, server->shutdown = 1
is directly set in the cases that matters, removed ret altogether.
  In the current code head, server->shutdown has been renamed
server->quitEventThread, so patch changed accordingly.

> At line 1535 seems to want to loop on reads that return -1 and either EINTR or
> EAGAIN. The implementation does not. Rather than calling return at line 1538,
> it should likely have a "goto again" with the again label placed just before
> the read call.

  Actually in that case I think the current code is fine, the function
should exit 0 on EAGAIN, it's documented, I assume the read will be
retried by the caller, otherwise we would have used saferead() there an
internal routine taking directly care of interrupted calls.

> At line 1783 is a call to write that likely wants a treatment similar to the
> read above.

  Same thing, I guess it's intended behaviour :-)

> In tools/virsh.c at line 4616 is a test that format is true. It was checked at
> line 4614 and nothing changed it.

  Ah right, harmless but fixed :-)

> At line 6810 memory is allocated. At the error exit at line 6824 it has not
> been freed.

   okay

> At line 7664 is a test for network==NULL. This will always be true.
> At line 7704 is a test for iface == NULL. This will always be true.
> At line 7741 is a test for pool==NULL. This will always be true.

  Okay, fixed, allows the test to fit in 80 columns too,
Patch for git head attached.

   thanks a lot !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/
-------------- next part --------------
commit e74427b0570f4389b77c035dc093312f90b3d1e1
Author: Daniel Veillard <veillard at redhat.com>
Date:   Tue Nov 10 14:40:22 2009 +0100

    Various fixes following a code review part 2
    
    * daemon/libvirtd.c tools/virsh.c: Steve Grubb <sgrubb at redhat.com> found
      a few more issues

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index daf06bc..2fcd9a9 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -359,7 +359,6 @@ qemudDispatchSignalEvent(int watch ATTRIBUTE_UNUSED,
                          void *opaque) {
     struct qemud_server *server = (struct qemud_server *)opaque;
     siginfo_t siginfo;
-    int ret;
 
     virMutexLock(&server->lock);
 
@@ -371,8 +370,6 @@ qemudDispatchSignalEvent(int watch ATTRIBUTE_UNUSED,
         return;
     }
 
-    ret = 0;
-
     switch (siginfo.si_signo) {
     case SIGHUP:
         VIR_INFO0(_("Reloading configuration on SIGHUP"));
@@ -392,9 +389,6 @@ qemudDispatchSignalEvent(int watch ATTRIBUTE_UNUSED,
         break;
     }
 
-    if (ret != 0)
-        server->quitEventThread = 1;
-
     virMutexUnlock(&server->lock);
 }
 
diff --git a/tools/virsh.c b/tools/virsh.c
index f8e6ce4..0d0ebca 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -4627,8 +4627,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
 
     if (format) {
         virBufferAddLit(&buf, "  <target>\n");
-        if (format)
-            virBufferVSprintf(&buf, "    <format type='%s'/>\n",format);
+        virBufferVSprintf(&buf, "    <format type='%s'/>\n",format);
         virBufferAddLit(&buf, "  </target>\n");
     }
     virBufferAddLit(&buf, "</volume>\n");
@@ -6835,6 +6834,7 @@ editWriteToTempFile (vshControl *ctl, const char *doc)
     if (fd == -1) {
         vshError(ctl, _("mkstemp: failed to create temporary file: %s"),
                  strerror(errno));
+        free (ret);
         return NULL;
     }
 
@@ -7675,7 +7675,7 @@ vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd,
         *name = n;
 
     /* try it by UUID */
-    if (network==NULL && (flag & VSH_BYUUID) && strlen(n)==VIR_UUID_STRING_BUFLEN-1) {
+    if ((flag & VSH_BYUUID) && (strlen(n) == VIR_UUID_STRING_BUFLEN-1)) {
         vshDebug(ctl, 5, "%s: <%s> trying as network UUID\n",
                  cmd->def->name, optname);
         network = virNetworkLookupByUUIDString(ctl->conn, n);
@@ -7715,7 +7715,7 @@ vshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd,
         *name = n;
 
     /* try it by NAME */
-    if ((iface == NULL) && (flag & VSH_BYNAME)) {
+    if ((flag & VSH_BYNAME)) {
         vshDebug(ctl, 5, "%s: <%s> trying as interface NAME\n",
                  cmd->def->name, optname);
         iface = virInterfaceLookupByName(ctl->conn, n);
@@ -7752,13 +7752,13 @@ vshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char *optname,
         *name = n;
 
     /* try it by UUID */
-    if (pool==NULL && (flag & VSH_BYUUID) && strlen(n)==VIR_UUID_STRING_BUFLEN-1) {
+    if ((flag & VSH_BYUUID) && (strlen(n) == VIR_UUID_STRING_BUFLEN-1)) {
         vshDebug(ctl, 5, "%s: <%s> trying as pool UUID\n",
                  cmd->def->name, optname);
         pool = virStoragePoolLookupByUUIDString(ctl->conn, n);
     }
     /* try it by NAME */
-    if (pool==NULL && (flag & VSH_BYNAME)) {
+    if (pool == NULL && (flag & VSH_BYNAME)) {
         vshDebug(ctl, 5, "%s: <%s> trying as pool NAME\n",
                  cmd->def->name, optname);
         pool = virStoragePoolLookupByName(ctl->conn, n);