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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@xxxxxxxxxx>
> Sent: 20 February 2020 16:20
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Liu <wl@xxxxxxx>; Anthony Perard
> <anthony.perard@xxxxxxxxxx>
> Subject: Re: [PATCH v6 1/6] libxl: add infrastructure to track and query
> 'recent' domids
> 
> Paul Durrant writes ("[PATCH v6 1/6] libxl: add infrastructure to track
> and query 'recent' domids"):
> > A domid is considered recent if the domain it represents was destroyed
> > less than a specified number of seconds ago. For debugging and/or
> testing
> > purposes the number can be set using the environment variable
> > LIBXL_DOMID_REUSE_TIMEOUT. If the variable does not exist then a default
> > value of 60s is used.
> ...
> 
> Quoting only the parts which are neither specific to the particular
> function, nor calls to the functions into which common code has
> currently been moved:
> 
> > +static int libxl__mark_domid_recent(libxl__gc *gc, uint32_t domid)
> > +{
> +    long timeout = libxl__get_domid_reuse_timeout();
> ...
> > +    if (clock_gettime(CLOCK_MONOTONIC, &ts)) {
> > +        LOGED(ERROR, domid, "failed to get time");
> > +        goto out;
> > +    }
> ...
> > +        if (ts.tv_sec - sec > timeout)
> > +            continue; /* Ignore expired entries */
> 
> > +int libxl__is_domid_recent(libxl__gc *gc, uint32_t domid, bool *recent)
> > +{
> > +    long timeout = libxl__get_domid_reuse_timeout();
> ...
> > +    if (clock_gettime(CLOCK_MONOTONIC, &ts)) {
> > +        LOGED(ERROR, domid, "failed to get time");
> > +        goto out;
> > +    }
> ...
> > +        if (val == domid && ts.tv_sec - sec <= timeout) {
> 
> I'm afraid I am still making style comments:
> 
> 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.

> 
> In your factored out functions you generally do this:
> 
>    int some_function(){
>       r = do_the_thing();
>       if (r == 0) return 0;
> 
>       LOGE(....)
>       return ERROR_FAIL;
>     }
> 
> This structure is not ideal because:
> 
>  - It makes it hard to extend this function to do more, later.
>    For example, refactoring the clock_gettime call into
>    what is now libxl__open_domid_history would involve reorganising
>    the function.

Ok, but it makes the code shorter done the way I have it and I don't really see 
why any necessary future re-organisation would be such a problem.

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

> 
>  - It is nonstandard.  See ERROR_HANDLING in CODING_STYLE.
> 
> > +    ret = fclose(nf);
> 
> This should be called `r', not `ret'.  See CODING_STYLE.

Ok, I clearly didn't pick up all the subtleties.

> 
> Sorry that some of the other code which you are having to edit here
> sets a bad example.  (See the apology at the top of CODING_STYLE.)
> (Existing uses of `ret' in libxl are sometimes a syscall return value
> and sometimes a libxl error code, which is one reason that name is now
> deprecated.)
> 
> > +static int libxl__replace_domid_history(libxl__gc *gc, char *new)
> > +{
> 
> For the record: it was not necessary to break this out into its own
> function, because there is only one call site, so open-coding it would
> not duplicate anything.  On the other hand if you think it is clearer,
> I have no objection.
> 
> I think the actual behaviour is correct now but I would like to read
> it again when it is in the conventional style.
> 

I will spin it again shortly.

  Paul

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