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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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.

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.

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.

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

 - It is nonstandard.  See ERROR_HANDLING in CODING_STYLE.

> +    ret = fclose(nf);

This should be called `r', not `ret'.  See CODING_STYLE.

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.

Thanks
Ian.

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