[Libguestfs] [PATCH] New APIs: hopen-device hopen-file hread hwrite hseek hclose hclose-all
Richard W.M. Jones
rjones at redhat.com
Thu Aug 26 12:52:43 UTC 2010
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
More information about the Libguestfs
mailing list