[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [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.

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;

      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

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


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.