[Libvir] [PATCH] Device attach/detach on virsh(XML version)
Mark McLoughlin
markmc at redhat.com
Wed May 23 09:09:23 UTC 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.
More information about the libvir-list
mailing list