Re: [Xen-devel] [PATCH 2/2] libxl: implement libxl__xs_mknod using XS_WRITE rather than XS_MKDIR

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx]
> Sent: 25 November 2015 14:04
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini; Ian Campbell; Wei Liu
> Subject: Re: [PATCH 2/2] libxl: implement libxl__xs_mknod using XS_WRITE
> rather than XS_MKDIR
> 
> Paul Durrant writes ("[PATCH 2/2] libxl: implement libxl__xs_mknod using
> XS_WRITE rather than XS_MKDIR"):
> > This patch modifies the implentation of libxl__xs_mknod() to use
> XS_WRITE
> > rather than XS_MKDIR since passing an empty value to the former will
> > ensure that the path is both existent and empty upon return, rather than
> > merely existent. The function return type is also changed to a libxl
> > error value rather than a boolean, it's declaration is accordingly moved
> > into the 'checked' section in libxl_internal.h, and a comment is added to
> > clarify its semantics.
> ...
> > +/* On success, path will exist and will be empty */
> > +int libxl__xs_mknod(libxl__gc *gc, xs_transaction_t t,
> > +                    const char *path, struct xs_permissions *perms,
> > +                    unsigned int num_perms);
> 
> I like the idea of making this function a `checked' one, but:
> 
> /*----- "checked" xenstore access functions -----*/
> /* Each of these functions will check that it succeeded; if it
>  * fails it logs and returns ERROR_FAIL.
>  */
> 
> but your implementation does not log anything.

Damn. I meant to add that too.

> 
> I think it's probably correct to add the logging but it would perhaps
> be worth checking the call sites.
> 

Sure.

> 
> While I am here, you maybe want to adjust the wording of a couple of
> comments:
> 
> > +/* On success, path will exist and will be empty */
> 
> You mean `will have an empty value'.  WRITE does not delete any
> subtrees (and libxl__xs_mknod shouldn't IMO).
> 

Yes, I'll re-word.

> 
> > +  bool ok;   /* the return value from a boolean function */
> 
> Maybe say `the success return value from a boolean function' ?
> 

Ok.

  Paul

> Ian.

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