[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH 1/4 version 2] Implement progress messages in the daemon and library.



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]