7:52 a.m.
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