Re: [Xen-devel] [PATCH 07/15] libxl: provide libxl__xs_*_checked and libxl__xs_transaction_*

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 07/15] libxl: provide 
libxl__xs_*_checked and libxl__xs_transaction_*"):
>  
> > +int libxl__xs_read_checked(libxl__gc *gc, xs_transaction_t t,
> > +                           const char *path, const char **result_out)
> > +{
> > +    char *result = libxl__xs_read(gc, t, path);
> > +    if (!result) {
> > +        if (errno != ENOENT) {
> 
> Can't you combine these with && ?

Yes, but this matches better the way I think of it.  I can change it
if you like.

> > +int libxl__xs_transaction_commit(libxl__gc *gc, xs_transaction_t *t)
> > +{
...
> > +        LOGE(ERROR, "could not commit xenstore transacton");
> 
>                                                   transaction

Oops.

> It would be much more useful if this function took a "const char
> *what" (even just __func__ from the caller) and logged it here.

I think that this call can only fail due to general failure of
xenstore (or our xenstore handle).  So a `what' here wouldn't help
much.  The other helpers are more likely to fail (permissions, for
example) but they can log the path.

I played about a bit with adding more information supplied by the
callers.  Ultimately what I seem to end up with is something that's
rather too clumsy to use without helper macros which I don't think
will assist with clarity.

Eg,

        if (got) {
            const char *got_ret;
            rc = XLXS_READ(t, lds->ret_path, &got_ret);
            if (rc) goto out;

which is all very well but hardly a model of clarity.

An alternative would be to replace `xs_transaction_t t' with a struct
that contains at least the file and function name and a `what', which
would be passed to the transaction start helper.

So I think I would prefer to keep things as they are.

Ian.

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