Re: [PATCH v9 02/15] libxc/progress: Extend the progress interface

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-04-20 at 14:15 +0100, Andrew Cooper wrote:
> On 15/04/15 11:55, Ian Campbell wrote:
> > On Fri, 2015-04-10 at 18:15 +0100, Andrew Cooper wrote:
> >> Not everything which needs reporting as progress comes with a range.  Extend
> >> the interface to allow reporting of a single statement.
> >>
> >> The programming interface now looks like:
> >>   xc_set_progress_prefix()
> >>     set the prefix string to be used
> >>   xc_report_progress_single()
> >>     report a single action
> >>   xc_report_progress_step()
> >>     report $X of $Y
> >>
> >> The new programming interface is implemented in a compatible way with the
> >> existing caller interface (by reporting a single action as "0 of 0").
> > I suppose the underlying motivation here is that there a difference
> > between this new xc_report_progress_step and calling xc_report/v? IOW
> > some difference between the semantics of the logger's ->vmessage and
> > ->progress hooks. What is it though?
> 
> They arrive via separate callbacks to the callee.
> 
> xc_report_progress & friends come through the domain build logger
> (parameter 2 of xc_interface_open()) while other logging tends to come
> through the regular logger (parameter 1 of xc_interface_open()).
> 
> Technically speaking, xc_report/v does allow the caller to choose which
> logger is used, but also requires the caller to provide all the fine
> detail, which detracts from the readability of the callsite.
> 
> This new progress API attempts to resolve this by providing a high level
> way of putting a single message through the progress logger.

Makes sense, thanks.

> > I suspected the distinction was in the automatic inclusion of
> > xch->currently_progress_reporting into the messages, but you appear to
> > make that non-mandatory below.
> >
> > Speaking of which, I think it should be mandatory now to call
> > xc_set_progress_prefix as it was to call progress_start before, and that
> > both of your new functions should assert. Those who think they want to
> > use xc_report_progress_single without calling xc_set_progress_prefix
> > should be using xc_report() instead.
> 
> Technically speaking it is safe to use a NULL prefix; the vsprintf won't
> fall over.

Having log message prefixed by "(null)" or whatever would be a bit
rubbish. I think lets keep the current semantics (by asserting things
have been set) and if there is a strong case to allow no prefix relax
things in a subsequent patch which makes everywhere DTRT.

> I can assert() that these invariants are held, but it is not critical to
> the functionality.

Yes, please add the asserts for now.

Ian.