On Fri, Feb 08, 2013 at 12:16:15PM +0000, Matthew Booth wrote:
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.
ACK with the changes discussed.
Rich.
--
Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top