[Libguestfs] [PATCH] New APIs: hopen-device hopen-file hread hwrite hseek hclose hclose-all

[Richard W.M. Jones]

On Thu, Aug 26, 2010 at 11:45:17AM +0100, Matthew Booth wrote:
> +    if (fd < 0) {

I believe that these error checks should be == -1 instead of < 0.
(There are several more below).

> +        reply_with_perror_errno(errno, "open %s failed", path);

Just use: reply_with_perror ("open failed: %s", path).
It's a macro which expands to the same as above.  There are several
more cases where the same thing is done.

> +            char *error = NULL;
> +            int len = asprintf(&error, "close handle %i failed: %m ", fd);
> +            if (len < 0) {
> +                reply_with_error("asprintf");
> +            }

Missing return statement.

> +            msg = realloc(msg, msglen + len);
> +            if (NULL == msg) {
> +                reply_with_error("realloc");
> +                return -1;
> +            }
> +
> +            memcpy(msg + msglen, error, len);
> +            msglen += len;
> +            free(error);
> +        }
> +    }
> +
> +    if (failed) {
> +        msg[msglen] = '\0';
> +        reply_with_error("%s", msg);
> +        return -1;
> +    }

Is there a way to do this without dynamic memory allocations?  How
about putting a fixed buffer on the stack which is the same length as
the maximum error message size (64K, so not too large for the stack):

  char errors[GUESTFS_ERROR_LEN];
  char *error_p = errors;
  size_t error_n = sizeof errors - 1;

  /* when you get an error ... */
  if (error_n > 0) {
    strncpy (error_p, strerror (errno), error_n);
    size_t n = strlen (errors);
    error_p = &errors[n];
    error_n = errors[sizeof errors] - error_p - 1;
  }

Or you could just return the first error ...

> +    return 0;
> +}
> +
> +char * /* RBufferOut */
> +do_hread (int handle, int64_t size, size_t *size_r)
> +{

Need to check that the handle is a member of the list.

You should also check that size < GUESTFS_MESSAGE_MAX, just to avoid
huge-but-not-fatal memory allocations at the next line.  See the
implementation of do_pread in daemon/file.c for an example.

> +    char *buf = malloc(size);
> +    if (NULL == buf) {
> +        reply_with_error("malloc");

reply_with_perror.

> +        goto error;

> +error:
> +    free(buf);
> +    return NULL;

But of a mouthful, just when we want to free a single value.  I
think the goto is unnecessary here.

> +int /* RErr */
> +do_hwrite (int handle, const char *content, size_t content_size)
> +{
> +    size_t pos = 0;
> +    while (pos < content_size) {
> +        ssize_t out = write(handle, content + pos, content_size - pos);
> +
> +        if (out < 0) {
> +            reply_with_perror_errno(errno, "error writing to handle %i", handle);
> +            return -1;
> +        }
> +        pos += out;
> +    }
> +
> +    return 0;
> +}

... and you were going to change do_pwrite as well.

> +# Create a 10M image file and add it to the handle
> +my ($fh, $path) = tempfile();
> +truncate($fh, 1024*1024*10);
> +close($fh) or die("close $path failed: $!");

In the other scripts we create a file in the current directory called
"test.img".  See regressions/test-lvm-mapping.pl for an example.

> +# As qemu is now running we can unlink the image
> +unlink($path) or die("unlink $path failed: $!");

... but unlinking the image early is a good idea.

> +my $string = "abcd";
> +
> +# Test read and writing directly to a block device
> +my $h = $g->hopen_device("/dev/vda", 2);
> +$g->hwrite($h, $string);
> +$g->hseek($h, 0, 0);
> +my $in = $g->hread($h, length($string));
> +
> +die("device: hread returned $in") unless($in eq $string);
> +$g->hclose($h);
> +
> +# Check $h is no longer valid
> +eval {
> +    $g->hseek($h, 0, 0);
> +};
> +die("device: handle wasn't closed") unless($@);

It's not really much of a test, writing and reading 4 bytes.

Aligned vs unaligned?
Seeking and writing in a large sparse file?
Writing past the end of a large file?
Writing past the end of a device?
Reading and writing large buffers (just below 2MB and over 4MB, the
latter should fail gracefully).

> +=over

=over 4
for consistency at least?

> +See L<guestfs_hopen_file> for a description of C<flags>.");

You should write:

  See C<guestfs_hopen_file> etc

> +This command writes all the data in C<content> to C<handle>.  It
> +returns an error if this fails.");

You don't need to say it returns an error if it fails.  It would be a
serious bug if it _didn't_ do this :=) and in any case the surrounding
boilerplate will tell the caller how to detect errors from the call.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top