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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx]
> Sent: 24 November 2015 16:49
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Ian Campbell; Wei Liu
> Subject: Re: [Xen-devel] [PATCH v2 2/3] libxl: check for underlying xenstore
> operation failure...
> 
> (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 ?
> 

Ah yes, that would work nicely.

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

I'm completely happy with that style. I'll fix accordingly.

  Paul

> 
> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel