Richard W.M. Jones
2010-Dec-01 14:01 UTC
[Libguestfs] [PATCH 0/5] Add progress notification to upload APIs
-- 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
Richard W.M. Jones
2010-Dec-01 14:03 UTC
[Libguestfs] [PATCH 1/5] protocol: Upload progress messages and optional arguments.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora -------------- next part -------------->From 3aa8182c3cc478bf723205f1a4dd84e160768448 Mon Sep 17 00:00:00 2001From: Richard W.M. Jones <rjones at redhat.com> Date: Wed, 1 Dec 2010 10:30:44 +0000 Subject: [PATCH 1/5] protocol: Upload progress messages and optional arguments. Two unrelated changes to the protocol to support progress messages during uploads, and optional arguments. Note that this makes an incompatible change to the protocol, and this is reflected in the protocol version field (3 -> 4). --- daemon/daemon.h | 2 ++ daemon/proto.c | 28 ++++++++++++++++++++++++++++ generator/generator_xdr.ml | 4 +++- src/guestfs.pod | 8 ++++++++ src/proto.c | 2 ++ 5 files changed, 43 insertions(+), 1 deletions(-) diff --git a/daemon/daemon.h b/daemon/daemon.h index 6e9788a..1e58910 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -103,6 +103,8 @@ extern const char *function_names[]; /*-- in proto.c --*/ extern int proc_nr; extern int serial; +extern uint64_t progress_hint; +extern uint64_t optargs_bitmask; /*-- in mount.c --*/ extern int root_mounted; diff --git a/daemon/proto.c b/daemon/proto.c index 63d1cc9..f3a3b26 100644 --- a/daemon/proto.c +++ b/daemon/proto.c @@ -22,6 +22,7 @@ #include <stdlib.h> #include <stdarg.h> #include <string.h> +#include <inttypes.h> #include <unistd.h> #include <errno.h> #include <sys/param.h> /* defines MIN */ @@ -45,6 +46,23 @@ int proc_nr; int serial; +/* Hint for implementing progress messages for uploaded/incoming data. + * The caller sets this to a value > 0 if it knows or can estimate how + * much data will be sent (this is not always known, eg. for uploads + * coming from a pipe). If this is known then we can emit progress + * messages as we write the data. + */ +uint64_t progress_hint; + +/* Optional arguments bitmask. Caller sets this to indicate which + * optional arguments in the guestfs_<foo>_args structure are + * meaningful. Optional arguments not covered by the bitmask are set + * to arbitrary values and the daemon should ignore them. If the + * bitmask has bits set that the daemon doesn't understand, then the + * whole call is rejected early in processing. + */ +uint64_t optargs_bitmask; + /* Time at which we received the current request. */ static struct timeval start_t; @@ -149,9 +167,19 @@ main_loop (int _sock) reply_with_error ("unexpected message status (%d)", hdr.status); goto cont; } + /* This version of the daemon does not understand optional arguments + * at all. When we fix this, we will remove the next conditional. + */ + if (hdr.optargs_bitmask != 0) { + reply_with_error ("optargs_bitmask != 0 (%" PRIu64 ")", + hdr.optargs_bitmask); + goto cont; + } proc_nr = hdr.proc; serial = hdr.serial; + progress_hint = hdr.progress_hint; + optargs_bitmask = hdr.optargs_bitmask; /* Clear errors before we call the stub functions. This is just * to ensure that we can accurately report errors in cases where diff --git a/generator/generator_xdr.ml b/generator/generator_xdr.ml index c6d8a4d..ca114c5 100644 --- a/generator/generator_xdr.ml +++ b/generator/generator_xdr.ml @@ -158,7 +158,7 @@ let generate_xdr () */ const GUESTFS_PROGRAM = 0x2000F5F5; -const GUESTFS_PROTOCOL_VERSION = 3; +const GUESTFS_PROTOCOL_VERSION = 4; /* These constants must be larger than any possible message length. */ const GUESTFS_LAUNCH_FLAG = 0xf5f55ff5; @@ -193,6 +193,8 @@ struct guestfs_message_header { guestfs_procedure proc; /* GUESTFS_PROC_x */ guestfs_message_direction direction; unsigned serial; /* message serial number */ + unsigned hyper progress_hint; /* upload hint for progress bar */ + unsigned hyper optargs_bitmask; /* bitmask for optional args */ guestfs_message_status status; }; diff --git a/src/guestfs.pod b/src/guestfs.pod index 677d944..7cb05a6 100644 --- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -1855,6 +1855,14 @@ The header contains the procedure number (C<guestfs_proc>) which is how the receiver knows what type of args structure to expect, or none at all. +For functions that take optional arguments, the optional arguments are +encoded in the C<guestfs_I<foo>_args> structure in the same way as +ordinary arguments. A bitmask in the header indicates which optional +arguments are meaningful. The bitmask is also checked to see if it +contains bits set which the daemon does not know about (eg. if more +optional arguments were added in a later version of the library), and +this causes the call to be rejected. + The reply message for ordinary functions is: total length (header + ret, diff --git a/src/proto.c b/src/proto.c index 5d924e8..5c22e3d 100644 --- a/src/proto.c +++ b/src/proto.c @@ -658,6 +658,8 @@ guestfs___send (guestfs_h *g, int proc_nr, xdrproc_t xdrp, char *args) hdr.direction = GUESTFS_DIRECTION_CALL; hdr.serial = serial; hdr.status = GUESTFS_STATUS_OK; + hdr.progress_hint = 0; + hdr.optargs_bitmask = 0; if (!xdr_guestfs_message_header (&xdr, &hdr)) { error (g, _("xdr_guestfs_message_header failed")); -- 1.7.3.2
Richard W.M. Jones
2010-Dec-01 14:03 UTC
[Libguestfs] [PATCH 2/5] protocol: Send progress_hint in header.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora -------------- next part -------------->From 7e523077d650cfb71044d5e74aa8fe1f91c019ae Mon Sep 17 00:00:00 2001From: Richard W.M. Jones <rjones at redhat.com> Date: Wed, 1 Dec 2010 13:28:26 +0000 Subject: [PATCH 2/5] protocol: Send progress_hint in header. For actions that have FileIn arguments, count the size of all the input files and send that in the progress_hint field of the request header. --- generator/generator_c.ml | 34 +++++++++++++++++++++++++++++++--- src/guestfs-internal.h | 2 +- src/proto.c | 5 +++-- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/generator/generator_c.ml b/generator/generator_c.ml index 449f748..d8b13a5 100644 --- a/generator/generator_c.ml +++ b/generator/generator_c.ml @@ -577,6 +577,9 @@ and generate_client_actions () #include <stdint.h> #include <string.h> #include <inttypes.h> +#include <unistd.h> +#include <sys/types.h> +#include <sys/stat.h> #include \"guestfs.h\" #include \"guestfs-internal.h\" @@ -960,10 +963,34 @@ check_state (guestfs_h *g, const char *caller) | RStructList (_, typ) -> pr " struct guestfs_%s_list *ret_v;\n" typ ); + + let has_filein + List.exists (function FileIn _ -> true | _ -> false) args in + if has_filein then ( + pr " uint64_t progress_hint = 0;\n"; + pr " struct stat progress_stat;\n"; + ) else + pr " const uint64_t progress_hint = 0;\n"; + pr "\n"; check_null_strings shortname style; reject_unknown_optargs shortname style; trace_call shortname style; + + (* Calculate the total size of all FileIn arguments to pass + * as a progress bar hint. + *) + List.iter ( + function + | FileIn n -> + pr " if (stat (%s, &progress_stat) == 0 &&\n" n; + pr " S_ISREG (progress_stat.st_mode))\n"; + pr " progress_hint += progress_stat.st_size;\n"; + pr "\n"; + | _ -> () + ) args; + + (* Check we are in the right state for sending a request. *) pr " if (check_state (g, \"%s\") == -1) {\n" shortname; trace_return_error ~indent:4 style; pr " return %s;\n" error_code; @@ -974,8 +1001,9 @@ check_state (guestfs_h *g, const char *caller) (* Send the main header and arguments. *) (match args with | [] -> - pr " serial = guestfs___send (g, GUESTFS_PROC_%s, NULL, NULL);\n" - (String.uppercase shortname) + pr " serial = guestfs___send (g, GUESTFS_PROC_%s, progress_hint,\n" + (String.uppercase shortname); + pr " NULL, NULL);\n" | args -> List.iter ( function @@ -1006,7 +1034,7 @@ check_state (guestfs_h *g, const char *caller) pr " args.%s.%s_len = %s_size;\n" n n n | Pointer _ -> assert false ) args; - pr " serial = guestfs___send (g, GUESTFS_PROC_%s,\n" + pr " serial = guestfs___send (g, GUESTFS_PROC_%s, progress_hint,\n" (String.uppercase shortname); pr " (xdrproc_t) xdr_%s_args, (char *) &args);\n" name; diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index d12e345..e4e198b 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -250,7 +250,7 @@ extern void guestfs___print_timestamped_message (guestfs_h *g, const char *fs, . extern void guestfs___free_inspect_info (guestfs_h *g); extern int guestfs___set_busy (guestfs_h *g); extern int guestfs___end_busy (guestfs_h *g); -extern int guestfs___send (guestfs_h *g, int proc_nr, xdrproc_t xdrp, char *args); +extern int guestfs___send (guestfs_h *g, int proc_nr, uint64_t progress_hint, xdrproc_t xdrp, char *args); extern int guestfs___recv (guestfs_h *g, const char *fn, struct guestfs_message_header *hdr, struct guestfs_message_error *err, xdrproc_t xdrp, char *ret); extern int guestfs___send_file (guestfs_h *g, const char *filename); extern int guestfs___recv_file (guestfs_h *g, const char *filename); diff --git a/src/proto.c b/src/proto.c index 5c22e3d..a2a5a15 100644 --- a/src/proto.c +++ b/src/proto.c @@ -627,7 +627,8 @@ guestfs___accept_from_daemon (guestfs_h *g) } int -guestfs___send (guestfs_h *g, int proc_nr, xdrproc_t xdrp, char *args) +guestfs___send (guestfs_h *g, int proc_nr, uint64_t progress_hint, + xdrproc_t xdrp, char *args) { struct guestfs_message_header hdr; XDR xdr; @@ -658,7 +659,7 @@ guestfs___send (guestfs_h *g, int proc_nr, xdrproc_t xdrp, char *args) hdr.direction = GUESTFS_DIRECTION_CALL; hdr.serial = serial; hdr.status = GUESTFS_STATUS_OK; - hdr.progress_hint = 0; + hdr.progress_hint = progress_hint; hdr.optargs_bitmask = 0; if (!xdr_guestfs_message_header (&xdr, &hdr)) { -- 1.7.3.2
Richard W.M. Jones
2010-Dec-01 14:04 UTC
[Libguestfs] [PATCH 3/5] protocol: Really read 4 bytes while checking for cancellation
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html -------------- next part -------------->From 8bfca99b9ab5774ce8aa1086184479ebb98236b2 Mon Sep 17 00:00:00 2001From: Richard W.M. Jones <rjones at redhat.com> Date: Wed, 1 Dec 2010 13:24:23 +0000 Subject: [PATCH 3/5] protocol: Really read 4 bytes while checking for cancellation. We've not actually hit this bug in practice, but at least in theory while checking for cancellation we could read > 0 but fewer than 4 bytes, which would effectively be discarded and we would lose synchronization. Note the socket is non-blocking. Change the code so that we temporarily set the socket back to blocking and force the read of all 4 bytes. --- src/proto.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/proto.c b/src/proto.c index a2a5a15..dd81f48 100644 --- a/src/proto.c +++ b/src/proto.c @@ -245,11 +245,56 @@ read_log_message_or_eof (guestfs_h *g, int fd, int error_if_eof) return 0; } +/* Read 'n' bytes, setting the socket to blocking temporarily so + * that we really read the number of bytes requested. + * Returns: 0 == EOF while reading + * -1 == error, error() function has been called + * n == read 'n' bytes in full + */ +static ssize_t +really_read_from_socket (guestfs_h *g, int sock, char *buf, size_t n) +{ + long flags; + ssize_t r; + size_t got; + + /* Set socket to blocking. */ + flags = fcntl (sock, F_GETFL); + if (flags == -1) { + perrorf (g, "fcntl"); + return -1; + } + if (fcntl (sock, F_SETFL, flags & ~O_NONBLOCK) == -1) { + perrorf (g, "fcntl"); + return -1; + } + + got = 0; + while (got < n) { + r = read (sock, &buf[got], n-got); + if (r == -1) { + perrorf (g, "read"); + return -1; + } + if (r == 0) + return 0; /* EOF */ + got += r; + } + + /* Restore original socket flags. */ + if (fcntl (sock, F_SETFL, flags) == -1) { + perrorf (g, "fcntl"); + return -1; + } + + return (ssize_t) got; +} + static int check_for_daemon_cancellation_or_eof (guestfs_h *g, int fd) { char buf[4]; - int n; + ssize_t n; uint32_t flag; XDR xdr; @@ -258,21 +303,15 @@ check_for_daemon_cancellation_or_eof (guestfs_h *g, int fd) "check_for_daemon_cancellation_or_eof: %p g->state = %d, fd = %d\n", g, g->state, fd); - n = read (fd, buf, 4); + n = really_read_from_socket (g, fd, buf, 4); + if (n == -1) + return -1; if (n == 0) { /* Hopefully this indicates the qemu child process has died. */ child_cleanup (g); return -1; } - if (n == -1) { - if (errno == EINTR || errno == EAGAIN) - return 0; - - perrorf (g, "read"); - return -1; - } - xdrmem_create (&xdr, buf, 4, XDR_DECODE); xdr_uint32_t (&xdr, &flag); xdr_destroy (&xdr); -- 1.7.3.2
Richard W.M. Jones
2010-Dec-01 14:04 UTC
[Libguestfs] [PATCH 4/5] protocol: Handle progress notification messages during FileIn.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw -------------- next part -------------->From 11be64049ba3ce36e1be297d2d6f54abca079742 Mon Sep 17 00:00:00 2001From: Richard W.M. Jones <rjones at redhat.com> Date: Wed, 1 Dec 2010 13:31:25 +0000 Subject: [PATCH 4/5] protocol: Handle progress notification messages during FileIn. If the daemon sends progress notification messages while we are uploading FileIn parameters, these are received in check_for_daemon_cancellation_or_eof. Modify this library function so that it turns these messages into callbacks. --- daemon/proto.c | 1 + src/proto.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/daemon/proto.c b/daemon/proto.c index f3a3b26..1fdb26c 100644 --- a/daemon/proto.c +++ b/daemon/proto.c @@ -425,6 +425,7 @@ receive_file (receive_cb cb, void *opaque) return 0; /* end of file */ } + /* Note that the callback can generate progress messages. */ if (cb) r = cb (opaque, chunk.data.data_val, chunk.data.data_len); else diff --git a/src/proto.c b/src/proto.c index dd81f48..a5d9d2b 100644 --- a/src/proto.c +++ b/src/proto.c @@ -70,6 +70,9 @@ #include "guestfs-internal-actions.h" #include "guestfs_protocol.h" +/* Size of guestfs_progress message on the wire. */ +#define PROGRESS_MESSAGE_SIZE 24 + /* This is the code used to send and receive RPC messages and (for * certain types of message) to perform file transfers. This code is * driven from the generated actions (src/actions.c). There @@ -316,6 +319,33 @@ check_for_daemon_cancellation_or_eof (guestfs_h *g, int fd) xdr_uint32_t (&xdr, &flag); xdr_destroy (&xdr); + /* Read and process progress messages that happen during FileIn. */ + if (flag == GUESTFS_PROGRESS_FLAG) { + char buf[PROGRESS_MESSAGE_SIZE]; + + n = really_read_from_socket (g, fd, buf, PROGRESS_MESSAGE_SIZE); + if (n == -1) + return -1; + if (n == 0) { + child_cleanup (g); + return -1; + } + + if (g->state == BUSY && g->progress_cb) { + guestfs_progress message; + + xdrmem_create (&xdr, buf, PROGRESS_MESSAGE_SIZE, XDR_DECODE); + xdr_guestfs_progress (&xdr, &message); + xdr_destroy (&xdr); + + g->progress_cb (g, g->progress_cb_data, + message.proc, message.serial, + message.position, message.total); + } + + return 0; + } + if (flag != GUESTFS_CANCEL_FLAG) { error (g, _("check_for_daemon_cancellation_or_eof: read 0x%x from daemon, expected 0x%x\n"), flag, GUESTFS_CANCEL_FLAG); @@ -418,9 +448,6 @@ guestfs___send_to_daemon (guestfs_h *g, const void *v_buf, size_t n) * will not see GUESTFS_PROGRESS_FLAG. */ -/* Size of guestfs_progress message on the wire. */ -#define PROGRESS_MESSAGE_SIZE 24 - int guestfs___recv_from_daemon (guestfs_h *g, uint32_t *size_rtn, void **buf_rtn) { -- 1.7.3.2
Richard W.M. Jones
2010-Dec-01 14:04 UTC
[Libguestfs] [PATCH 5/5] Add progress notification messages to upload and upload-offset APIs.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/ -------------- next part -------------->From 8022d46e5e2d9c3ab664ace6c9f185976e34dc20 Mon Sep 17 00:00:00 2001From: Richard W.M. Jones <rjones at redhat.com> Date: Wed, 1 Dec 2010 13:33:31 +0000 Subject: [PATCH 5/5] Add progress notification messages to upload and upload-offset APIs. --- daemon/upload.c | 39 ++++++++++++++++++++++++++++----------- generator/generator_actions.ml | 4 ++-- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/daemon/upload.c b/daemon/upload.c index c026af8..e28bf96 100644 --- a/daemon/upload.c +++ b/daemon/upload.c @@ -31,25 +31,42 @@ #include "daemon.h" #include "actions.h" +struct write_cb_data { + int fd; /* file descriptor */ + uint64_t written; /* bytes written so far */ +}; + static int -write_cb (void *fd_ptr, const void *buf, size_t len) +write_cb (void *data_vp, const void *buf, size_t len) { - int fd = *(int *)fd_ptr; - return xwrite (fd, buf, len); + struct write_cb_data *data = data_vp; + int r; + + r = xwrite (data->fd, buf, len); + if (r == -1) + return -1; + + data->written += len; + + if (progress_hint > 0) + notify_progress (data->written, progress_hint); + + return 0; } /* Has one FileIn parameter. */ static int upload (const char *filename, int flags, int64_t offset) { - int err, fd, r, is_dev; + struct write_cb_data data = { .written = 0 }; + int err, r, is_dev; is_dev = STRPREFIX (filename, "/dev/"); if (!is_dev) CHROOT_IN; - fd = open (filename, flags, 0666); + data.fd = open (filename, flags, 0666); if (!is_dev) CHROOT_OUT; - if (fd == -1) { + if (data.fd == -1) { err = errno; r = cancel_receive (); errno = err; @@ -58,7 +75,7 @@ upload (const char *filename, int flags, int64_t offset) } if (offset) { - if (lseek (fd, offset, SEEK_SET) == -1) { + if (lseek (data.fd, offset, SEEK_SET) == -1) { err = errno; r = cancel_receive (); errno = err; @@ -67,22 +84,22 @@ upload (const char *filename, int flags, int64_t offset) } } - r = receive_file (write_cb, &fd); + r = receive_file (write_cb, &data.fd); if (r == -1) { /* write error */ err = errno; r = cancel_receive (); errno = err; if (r != -2) reply_with_error ("write error: %s", filename); - close (fd); + close (data.fd); return -1; } if (r == -2) { /* cancellation from library */ - close (fd); + close (data.fd); /* Do NOT send any error. */ return -1; } - if (close (fd) == -1) { + if (close (data.fd) == -1) { err = errno; if (r == -1) /* if r == 0, file transfer ended already */ r = cancel_receive (); diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml index 0d08f73..a405fd4 100644 --- a/generator/generator_actions.ml +++ b/generator/generator_actions.ml @@ -2359,7 +2359,7 @@ Reread the partition table on C<device>. This uses the L<blockdev(8)> command."); - ("upload", (RErr, [FileIn "filename"; Dev_or_Path "remotefilename"], []), 66, [], + ("upload", (RErr, [FileIn "filename"; Dev_or_Path "remotefilename"], []), 66, [Progress], [InitScratchFS, Always, TestOutput ( (* Pick a file from cwd which isn't likely to change. *) [["mkdir"; "/upload"]; @@ -5510,7 +5510,7 @@ removes the partition number, returning the device name The named partition must exist, for example as a string returned from C<guestfs_list_partitions>."); - ("upload_offset", (RErr, [FileIn "filename"; Dev_or_Path "remotefilename"; Int64 "offset"], []), 273, [], + ("upload_offset", (RErr, [FileIn "filename"; Dev_or_Path "remotefilename"; Int64 "offset"], []), 273, [Progress], (let md5 = Digest.to_hex (Digest.file "COPYING.LIB") in [InitScratchFS, Always, TestOutput ( [["upload_offset"; "../COPYING.LIB"; "/upload_offset"; "0"]; -- 1.7.3.2
Reasonably Related Threads
- [PATCH febootstrap 0/8] Add support for building an ext2-based appliance
- [PATCH 0/5] 5 conservative changes to errno handling
- [PATCH 0/9] Enhance virt-resize so it can really expand Linux and Windows guests
- [PATCH 0/8 v2 DISCUSSION ONLY] Connecting to live virtual machines
- [PATCH 0/7] Add libvirt domain to core API