Re: [Xen-devel] [PATCH 05/29] libxc/progress: Repurpose the current progress reporting infrastructure

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-09-10 at 18:10 +0100, Andrew Cooper wrote:
> Not everything which needs reporting as progress comes with a range.

Please can you expand on your reasoning wrt the places where you are
removing a usage of start with a size. Especially since you aren't
changing any subsequent progress_step call to a you new singleton
variant.

> Allow reporting "0 of 0" for a single progress statement.

Can we not arrange to suppress this entirely? Perhaps by turning total=0
into percent=-1 and having the lower level code omit that bit of the
message in that case? If doing that you should update xentoollog.h to
make this clear.

It appears you are also doing more than just this as well, by changing
start to set for some reason. Even if you want to change the parameters
it's not clear that the new name is in any way an improvement.

Thirdly you appear to also be arranging to make it allowable to call
progress_step without having previously called progress_start/set. Why
is this needed?

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> ---
>  tools/libxc/xc_domain_restore.c |    2 +-
>  tools/libxc/xc_domain_save.c    |    2 +-
>  tools/libxc/xc_private.c        |   22 ++++++++++++++--------
>  tools/libxc/xc_private.h        |    4 ++--
>  tools/libxc/xtl_core.c          |    9 +++++----
>  5 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index b9a56d5..b411126 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -1610,7 +1610,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>          goto out;
>      }
>  
> -    xc_report_progress_start(xch, "Reloading memory pages", dinfo->p2m_size);
> +    xc_report_progress_set(xch, "Reloading memory pages");
>  
>      /*
>       * Now simply read each saved frame into its new machine frame.
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 254fdb3..02544f8 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -1127,7 +1127,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, 
> uint32_t dom, uint32_t max_iter
>                   "Saving memory: iter %d (last sent %u skipped %u)",
>                   iter, sent_this_iter, skip_this_iter);
>  
> -        xc_report_progress_start(xch, reportbuf, dinfo->p2m_size);
> +        xc_report_progress_set(xch, reportbuf);
>  
>          iter++;
>          sent_this_iter = 0;
> diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
> index 0941b06..45537af 100644
> --- a/tools/libxc/xc_private.c
> +++ b/tools/libxc/xc_private.c
> @@ -388,18 +388,24 @@ void xc_osdep_log(xc_interface *xch, xentoollog_level 
> level, int code, const cha
>      va_end(args);
>  }
>  
> -void xc_report_progress_start(xc_interface *xch, const char *doing,
> -                              unsigned long total) {
> +const char *xc_report_progress_set(xc_interface *xch, const char *doing)
> +{
> +    const char *old = xch->currently_progress_reporting;
> +
>      xch->currently_progress_reporting = doing;
> -    xtl_progress(xch->error_handler, "xc", xch->currently_progress_reporting,
> -                 0, total);
> +    return old;
> +}
> +
> +void xc_report_progress_single(xc_interface *xch, const char *doing)
> +{
> +    xtl_progress(xch->error_handler, "xc", doing, 0, 0);
>  }
>  
>  void xc_report_progress_step(xc_interface *xch,
> -                             unsigned long done, unsigned long total) {
> -    assert(xch->currently_progress_reporting);
> -    xtl_progress(xch->error_handler, "xc", xch->currently_progress_reporting,
> -                 done, total);
> +                             unsigned long done, unsigned long total)
> +{
> +    xtl_progress(xch->error_handler, "xc",
> +                 xch->currently_progress_reporting ?: "???", done, total);
>  }
>  
>  int xc_get_pfn_type_batch(xc_interface *xch, uint32_t dom,
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 97e4a56..22021e9 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -119,8 +119,8 @@ void xc_report(xc_interface *xch, xentoollog_logger *lg, 
> xentoollog_level,
>                 int code, const char *fmt, ...)
>       __attribute__((format(printf,5,6)));
>  
> -void xc_report_progress_start(xc_interface *xch, const char *doing,
> -                              unsigned long total);
> +const char *xc_report_progress_set(xc_interface *xch, const char *doing);
> +void xc_report_progress_single(xc_interface *xch, const char *doing);
>  void xc_report_progress_step(xc_interface *xch,
>                               unsigned long done, unsigned long total);
>  
> diff --git a/tools/libxc/xtl_core.c b/tools/libxc/xtl_core.c
> index 326b97e..73add92 100644
> --- a/tools/libxc/xtl_core.c
> +++ b/tools/libxc/xtl_core.c
> @@ -66,13 +66,14 @@ void xtl_log(struct xentoollog_logger *logger,
>  void xtl_progress(struct xentoollog_logger *logger,
>                    const char *context, const char *doing_what,
>                    unsigned long done, unsigned long total) {
> -    int percent;
> +    int percent = 0;
>  
>      if (!logger->progress) return;
>  
> -    percent = (total < LONG_MAX/100)
> -        ? (done * 100) / total
> -        : done / ((total + 99) / 100);
> +    if ( total )
> +        percent = (total < LONG_MAX/100)
> +            ? (done * 100) / total
> +            : done / ((total + 99) / 100);
>  
>      logger->progress(logger, context, doing_what, percent, done, total);
>  }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel