Re: [Libguestfs] [PATCH 03/12] mountable: Implement Mountable support for all apis which take it
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).
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).
+ if (strncmp ((string), "btrfsvol:", strlen
("btrfsvol:")) == 0) { \
Better to use STRPREFIX here.
+ RESOLVE_DEVICE((string), cancel_stmt, fail_stmt);
\
Do we need parens around the *_stmts? (I'm not sure ...)
@@ -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.
The rest looks fine.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v