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