[Libguestfs] [PATCH 1/4 version 2] Implement progress messages in the daemon and library.
Matthew Booth
mbooth at redhat.com
Tue Aug 31 10:20:42 UTC 2010
On 28/08/10 13:16, Richard W.M. Jones wrote:
>> From 978997bb41015eb85f009d4d48ca1716c84d9a50 Mon Sep 17 00:00:00 2001
> From: Richard Jones<rjones at 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
More information about the Libguestfs
mailing list