Re: [Xen-devel] [PATCH v6 1/6] libxl: add infrastructure to track and query 'recent' domids

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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...

> >  - 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).

Thanks,
Ian.

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