Re: [PATCH v2 2/3] libxl: check for underlying xenstore operation failure...

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

(Added CC to Ian C and Wei.)

Paul Durrant writes ("[Xen-devel] [PATCH v2 2/3] libxl: check for underlying xenstore operation failure..."):
> ...in libxl__xs_writev_perms() and libxl__xs_printf()

Thanks for looking at this.


> ERROR_FAIL is returned when any underlying operation fails. This is a
> semantic change in the case of a vasprintf() failure in libxl__xs_printf(),
> but appears to be better than returning a hardcoded -1.

This is what libxl__alloc_failed is for.  But, surely it would be
better to replace the call to vasprintf with one to libxl__vsprintf ?

>          path = libxl__sprintf(gc, "%s/%s", dir, kvs[i]);
>          if (path && kvs[i + 1]) {
>              int length = strlen(kvs[i + 1]);
> -            xs_write(ctx->xsh, t, path, kvs[i + 1], length);
> -            if (perms)
> -                xs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> +            if (!xs_write(ctx->xsh, t, path, kvs[i + 1], length))
> +                return ERROR_FAIL;

This is a good change semantically but I'm not keen on this error
handling style.  CODING_STYLE says:

  * Function calls which might fail (ie most function calls) are
    handled by putting the return/status value into a variable, and
    then checking it in a separate statement:
            char *dompath = libxl__xs_get_dompath(gc, bl->domid);
            if (!dompath) { rc = ERROR_FAIL; goto out; }

In this case the libxenstore return value variable should be called
`r', I think.  CODING_STYLE says:

    int r;     /* the return value from a system call (or libxc call) */

> +            if (perms &&
> +                !xs_set_permissions(ctx->xsh, t, path, perms, num_perms))
> +                return ERROR_FAIL;

And this is worse, I'm afraid.  Nested ifs are not expensive and there
is no risk of overloading the compiler.  So I would like to see this:

  if (perms) {
      r = xs_set ...
      if (r) ...
  }


I won't insist on you changing the error exits to use `goto out',
although I tend to think that would be better.


Thanks,
Ian.