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

[Mark McLoughlin <markmc@xxxxxxxxxx>]

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.