From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v9 02/15] libxc/progress: Extend the progress interface Date: Wed, 22 Apr 2015 16:38:59 +0100 Message-ID: <1429717139.30934.46.camel@citrix.com> References: <1428686167-8279-1-git-send-email-andrew.cooper3@citrix.com> <1428686167-8279-3-git-send-email-andrew.cooper3@citrix.com> <1429095313.15516.193.camel@citrix.com> <5534FBDA.2070201@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5534FBDA.2070201@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Wei Liu , Ian Jackson , Xen-devel List-Id: xen-devel@lists.xenproject.org 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.