Re: [Libvir] [PATCH] Device attach/detach on virsh(XML version)

23 May 2007

      Hi Rich,

On Tue, 2007-05-22 at 15:10 +0100, Richard W.M. Jones wrote:
...
+static char *
+readFile (vshControl *ctl, const char *filename)
+{
+    char *buffer = NULL, *oldbuffer;
+    int len = 0, fd, r;
+    char b[1024];
+
+    fd = open (filename, O_RDONLY);
+    if (fd == -1) {
+    file_error:
+        vshError (ctl, FALSE, "%s: %s", filename, strerror (errno));
+    error:
+        if (buffer) free (buffer);
+        if (fd >= 0) close (fd);
+        return NULL;
+    }
+
+    for (;;) {
+        r = read (fd, b, sizeof b);
+        if (r == -1) goto file_error;
+        if (r == 0) break;      /* End of file. */
+        oldbuffer = buffer;
+        buffer = realloc (buffer, len+r);
+        if (buffer == NULL) {
+        out_of_memory:
+            vshError (ctl, FALSE, "realloc: %s", strerror (errno));
+            goto error;
+        }
+        memcpy (buffer+len, b, r);
+        len += r;
+    }
+
+    buffer = realloc (buffer, len+1);
+    if (buffer == NULL) goto out_of_memory;
+    buffer[len] = '\0';
+    return buffer;
+}
Truly, the way you're using gotos here gives me a serious headache.
Following the logic of the function involves jumping back and forth
around the code way too much. I noticed you using that style in the
remote patch too.

	I'd suggest something more like:

static char *
readFile(vshControl *ctl, const char *filename)
{
    char *retval;
    int len = 0, fd;

    if ((fd = open(filename, O_RDONLY)) < 0) {
        vshError (ctl, FALSE, "Failed to open '%s': %s",
                  filename, strerror (errno));
        return NULL;
    }

    if (!(retval = malloc(len + 1)))
        goto out_of_memory;

    while (1) {
        char buffer[1024];
        char *new;
        int ret;

        if ((ret = read(fd, buffer, sizeof(buffer))) == 0)
            break;

        if (ret == -1) {
            if (errno == EINTR)
                continue;

            vshError (ctl, FALSE, "Failed to open '%s': %s",
                      filename, strerror (errno));
            goto error;
        }

        if (!(new = realloc(retval, len + ret + 1)))
            goto out_of_memory;

        retval = new;

        memcpy(retval + len, buffer, ret);
        len += ret;
   }

   retval[len] = '\0';

   return buffer;

 out_of_memory:
   vshError (ctl, FALSE, "Error allocating memory: %s", 
             strerror(errno));

 error:
   if (retval)
     free(retval);
   close(fd);

   return NULL;
}

	You can read this version straight through without having to jump back
to an earlier part of the function.

Cheers,
Mark.

Re: [Libvir] [PATCH] Device attach/detach on virsh(XML version)

Mark McLoughlin