All of lore.kernel.org

* Re: [Xen-devel] [PATCH v6 1/6] libxl: add infrastructure to track and query 'recent' domids
From: Durrant, Paul @ 2020-02-20 16:54 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Anthony Perard, xen-devel@lists.xenproject.org, Wei Liu
In-Reply-To: <24142.47029.435605.456811@mariner.uk.xensource.com>

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 20 February 2020 16:46
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>
> Subject: RE: [PATCH v6 1/6] libxl: add infrastructure to track and query
> 'recent' domids
> 
> Durrant, Paul writes ("RE: [PATCH v6 1/6] libxl: add infrastructure to
> track and query 'recent' domids"):
> > [Ian:]
> > > IMO the reuse timeout call and the clock_gettime call should be put in
> > > libxl__open_domid_history; and the time filtering check should be
> > > folded into libxl__read_recent.
> >
> > Ok. I was having a hard time guessing just exactly what you're looking
> for. I think that makes it a little clearer.
> ...
> > > In my review of v4 I wrote:
> > >
> > >   Do you think this can be combined somehow ?  Possibilities seem to
> > >   include: explicit read_recent_{init,entry,finish} functions; a
> single
> > >   function with a "per-entry" callback; same but with a macro.  If you
> > >   do that you can also have it have handle the "file does not exist"
> > >   special case.
> > >
> > > You've provided the read_recent_entry function but the "init" function
> > > libxl__open_domid_history does too little.  This series seems to be
> > > moving towards what I called read_recent_{init,entry,finish} (which
> > > should probably have the timestamp and FILE* in a struct together) but
> > > it seems to be doing so quite slowly.
> >
> > Now again I'm not sure *exactly* what you want me to do, but I'll have
> another guess.
> 
> Maybe it would be better for us to try to come to a shared
> understanding before you do another respin...
> 

Not being co-located makes this somewhat tricky; I think it will basically still come down to me writing some code and then you saying whether that's what you meant... unless you can write some (pseudo-)code to illustrate? I think, from what you say below, I might now have a better idea of what you want so let's have one more go-around of me writing the code first :-)

> > >  - It encourages vacuous log messages whose content is mainly in the
> > >    function and line number framing:
> > >        +    LOGE(ERROR, "failed");
> > >        +    return ERROR_FAIL;
> > >        +}
> > >    rather than
> > >        if (!*f) {
> > >            LOGE(ERROR, "failed to open recent domid file `%s'", path);
> > >            rc = ERROR_FAIL;
> > >            goto out;
> > >        }
> > >    (and the latter is to be preferred).
> >
> > But by asking me to put the error handling inside the sub-functions I
> lost context such as 'path' which was present in the previous structure. I
> could pass in an argument purely for the benefit of a log function but
> that seems excessive. I guess I will pull the error logging out again, but
> that seemed to be against your requirement to de-duplicate code.
> 
> I think the path needs to be passed into these functions.  This is why
> I think the functions need to take a struct* as an argument, for their
> shared state (including the path and the other stuff).
> 

Ok, if that's the style you prefer I'll re-structure it that way.

  Paul

> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply