On 28/08/10 13:16, Richard W.M. Jones wrote:

...

From 978997bb41015eb85f009d4d48ca1716c84d9a50 Mon Sep 17 00:00:00 2001 From: Richard Jones<rjones@redhat.com> Date: Sat, 28 Aug 2010 10:33:24 +0100 Subject: [PATCH 1/4] Implement progress messages in the daemon and library.

This implements progress notification messages in the daemon, and adds a callback in the library to handle them.

No calls are changed so far, so in fact no progress messages can be generated by this commit.

For more details, see: https://www.redhat.com/archives/libguestfs/2010-July/msg00003.html https://www.redhat.com/archives/libguestfs/2010-July/msg00024.html --- daemon/daemon.h | 7 ++++ daemon/proto.c | 88 +++++++++++++++++++++++++++++++++++++++++++++--- src/generator.ml | 21 +++++++++++- src/guestfs-internal.h | 2 + src/guestfs.c | 8 ++++ src/guestfs.h | 5 ++- src/guestfs.pod | 57 +++++++++++++++++++++++++++++++ src/proto.c | 37 ++++++++++++++++++-- 8 files changed, 215 insertions(+), 10 deletions(-)

--- a/src/guestfs.pod +++ b/src/guestfs.pod @@ -1186,6 +1186,63 @@ languages (eg. if your HLL interpreter has already been cleaned up by the time this is called, and if your callback then jumps into some HLL function).

+=head2 guestfs_set_progress_callback + + typedef void (*guestfs_progress_cb) (guestfs_h *g, void *opaque, + int proc_nr, int serial, + uint64_t position, uint64_t total); + void guestfs_set_progress_callback (guestfs_h *g, + guestfs_progress_cb cb, + void *opaque); + +Some long-running operations can generate progress messages. If +this callback is registered, then it will be called each time a +progress message is generated (usually one second after the +operation started, and every second thereafter until it completes, +although the frequency may change in future versions). + +The callback receives two numbers: C<position> and C<total>. +The units of C<total> are not defined, although for some +operations C<total> may relate in some way to the amount of +data to be transferred (eg. in bytes or megabytes), and +C<position> may be the proportion which has been transferred.

s/proportion/(portion|amount|total)/

+The only defined and stable parts of the API are: + +=over 4 + +=item * + +The callback can display to the user some type of progress bar or +indicator which shows the ratio of C<position>:C<total>. + +=item * + +C<total> will be the same number for each progress message within +a single operation.

Is this restriction truly necessary? This api would be useful and well-defined with just the first and third restrictions. Allowing total to change over time is useful for estimated durations.

+=item * + +0 E<lt>= C<position> E<lt>= C<total> + +=item * + +If any progress notification is sent during a call, then a final +progress notification is always sent when C<position> = C<total>. + +This is to simplify caller code, so callers can easily set the +progress indicator to "100%" at the end of the operation, without +requiring special code to detect this case. + +=back + +The callback also receives the procedure number and serial number of +the call. These are only useful for debugging protocol issues, and +the callback can normally ignore them. The callback may want to +print these numbers in error messages or debugging messages. + +Progress callbacks only happen in the BUSY state. + =head1 BLOCK DEVICE NAMING

In the kernel there is now quite a profusion of schemata for naming diff --git a/src/proto.c b/src/proto.c index ad173c6..f9b5a33 100644 --- a/src/proto.c +++ b/src/proto.c @@ -373,17 +373,26 @@ guestfs___send_to_daemon (guestfs_h *g, const void *v_buf, size_t n) * * It also checks for EOF (qemu died) and passes that up through the * child_cleanup function above. + * + * Progress notifications are handled transparently by this function. + * If the callback exists, it is called. The caller of this function + * will not see GUESTFS_PROGRESS_FLAG. */ int guestfs___recv_from_daemon (guestfs_h *g, uint32_t *size_rtn, void **buf_rtn) { fd_set rset, rset2;

+#define PROGRESS_MESSAGE_SIZE 24

We really should be calculating this value automatically somehow. Is there any structure we can measure the size of?

+#define message_size() \ + (*size_rtn != GUESTFS_PROGRESS_FLAG ? *size_rtn : PROGRESS_MESSAGE_SIZE) + if (g->verbose) fprintf (stderr, "recv_from_daemon: %p g->state = %d, size_rtn = %p, buf_rtn = %p\n", g, g->state, size_rtn, buf_rtn);

+ again: FD_ZERO (&rset);

FD_SET (g->fd[1],&rset); /* Read qemu stdout for log messages& EOF. */ @@ -400,7 +409,7 @@ guestfs___recv_from_daemon (guestfs_h *g, uint32_t *size_rtn, void **buf_rtn) */ ssize_t nr = -4;

- while (nr< (ssize_t) *size_rtn) { + while (nr< (ssize_t) message_size()) {

That loop test is way too complex. This could be rewritten as: for (;;) { ssize_t message_size = *size_rtn != GUESTFS_PROGRESS_FLAG ? *size_rtn : PROGRESS_MESSAGE_SIZE; if (nr < message_size) break; This would also avoid the unsightly #define, be a bit nicer to the compiler, and re-use the value below.

rset2 = rset; int r = select (max_fd+1,&rset2, NULL, NULL, NULL); if (r == -1) { @@ -463,6 +472,8 @@ guestfs___recv_from_daemon (guestfs_h *g, uint32_t *size_rtn, void **buf_rtn) } else if (*size_rtn == GUESTFS_CANCEL_FLAG) return 0; + else if (*size_rtn == GUESTFS_PROGRESS_FLAG) + /*FALLTHROUGH*/; /* If this happens, it's pretty bad and we've probably lost * synchronization. */ @@ -473,11 +484,11 @@ guestfs___recv_from_daemon (guestfs_h *g, uint32_t *size_rtn, void **buf_rtn) }

/* Allocate the complete buffer, size now known. */ - *buf_rtn = safe_malloc (g, *size_rtn); + *buf_rtn = safe_malloc (g, message_size()); /*FALLTHROUGH*/ }

- size_t sizetoread = *size_rtn - nr; + size_t sizetoread = message_size() - nr; if (sizetoread> BUFSIZ) sizetoread = BUFSIZ;

r = read (g->sock, (char *) (*buf_rtn) + nr, sizetoread); @@ -524,6 +535,26 @@ guestfs___recv_from_daemon (guestfs_h *g, uint32_t *size_rtn, void **buf_rtn) } #endif

+ if (*size_rtn == GUESTFS_PROGRESS_FLAG) { + if (g->state == BUSY&& g->progress_cb) { + guestfs_progress message; + XDR xdr; + xdrmem_create (&xdr, *buf_rtn, 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); + } + + free (*buf_rtn); + *buf_rtn = NULL; + + /* Process next message. */ + goto again;

This is nasty. You could just return guestfs___recv_from_daemon, called recursively. I think gcc is pretty efficient with the stack in this case.

+ } + return 0; }

-- 1.7.1

-- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490