Re: [Libguestfs] [PATCH 03/12] mountable: Implement Mountable support for all apis which take it

On Fri, 2013-02-08 at 11:56 +0000, Richard W.M. Jones wrote: > On Thu, Feb 07, 2013 at 03:57:49PM +0000, Matthew Booth wrote: > > A Mountable is passed from the library to the daemon as a string. The daemon > > stub parses it into a mountable_t, which it passes to the implementation. > > > > Update all implementations which now take a mountable_t. > > --- > > daemon/blkid.c | 33 +++++++++++++++++++++++----- > > daemon/daemon.h | 41 ++++++++++++++++++++++++++++++++++ > > daemon/ext2.c | 15 ++++++++++--- > > daemon/guestfsd.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > > daemon/labels.c | 16 ++++++++++++-- > > daemon/mount.c | 49 +++++++++++++++++++++++++++++++++-------- > > generator/c.ml | 12 +++++++++- > > generator/daemon.ml | 11 +++++++--- > > 8 files changed, 213 insertions(+), 27 deletions(-) > > > > diff --git a/daemon/blkid.c b/daemon/blkid.c > > index 64919dd..d54ac22 100644 > > --- a/daemon/blkid.c > > +++ b/daemon/blkid.c > > @@ -67,21 +67,42 @@ get_blkid_tag (const char *device, const char *tag) > > } > > > > char * > > -do_vfs_type (const char *device) > > +do_vfs_type (const mountable_t *mountable) > > { > [...] > > + default: > > + reply_with_error ("Invalid mountable type: %i", mountable->type); > > + return NULL; > > + } > > } > > Lots of this code has these error cases. However it would be much > simpler if the error case was handled by the generator, and my reading > of RESOLVE_MOUNTABLE is that it already *is* handled by the generator. > So you could assume that mountable->type would never be anything else > and replace the 'default' case with a call to abort () (even better if > GCC gave us a way to say the default case never happens, but that > doesn't seem to be possible). They're certainly belt and braces. I'm happy to take them out. > There's even a possibility that these error cases are wrong because > they call reply_with_error twice along the error path (which results > in protocol desynchronization). I don't think so. I've was pretty careful about errors on error paths (see extensive use of fprintf). > > + if (strncmp ((string), "btrfsvol:", strlen ("btrfsvol:")) == 0) { \ > > Better to use STRPREFIX here. ACK. > > > + RESOLVE_DEVICE((string), cancel_stmt, fail_stmt); \ > > Do we need parens around the *_stmts? (I'm not sure ...) I don't know either. I think I was copying other usage, tbh. I'm happy to change this or not. > > @@ -257,7 +262,7 @@ and generate_daemon_actions () = > > (function FileIn _ | FileOut _ -> false | _ -> true) args in > > let style = ret, args' @ args_of_optargs optargs, [] in > > pr " r = do_%s " name; > > - generate_c_call_args style; > > + generate_c_call_args ~in_daemon:true style; > > Is this a bug in existing code? It may need to be separately > backported if so. No, I added in_daemon to generate_c_call_args specifically for mountable support. The reason is that in the library the call is passed a string, but in the daemon it's passed a parsed mountable, so it needs to differentiate.