Re: [Libguestfs] [PATCH 2/6] btrfs: add optional parameter `ro' to btrfs_subvolume_snapshot

On Friday 21 November 2014 13:17:56 Hu Tao wrote: > Parameter `ro' is for creating readonly btrfs snapshot. > > Signed-off-by: Hu Tao <hutao(a)cn.fujitsu.com&gt; > --- > daemon/btrfs.c | 11 ++++++++++- > generator/actions.ml | 4 ++-- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 7a4d43d..b630bce 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -208,7 +208,7 @@ do_mkfs_btrfs (char *const *devices, > } > > int > -do_btrfs_subvolume_snapshot (const char *source, const char *dest) > +do_btrfs_subvolume_snapshot (const char *source, const char *dest, > int ro) { > const size_t MAX_ARGS = 64; > const char *argv[MAX_ARGS]; > @@ -231,6 +231,15 @@ do_btrfs_subvolume_snapshot (const char *source, > const char *dest) ADD_ARG (argv, i, str_btrfs); > ADD_ARG (argv, i, "subvolume"); > ADD_ARG (argv, i, "snapshot"); > + > + /* Optional arguments. */ > + if (!(optargs_bitmask & > GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK)) + ro = 0; > + > + if (ro) { > + ADD_ARG (argv, i, "-r"); > + } > + I would make this bit simplier: if ((optargs_bitmask & GUESTFS_BTRFS_SUBVOLUME_SNAPSHOT_RO_BITMASK) && ro) ADD_ARG (argv, i, "-r"); Also, seen in the other patches as well, you don't need { ... } brackets for blocks of just one instructions.

> diff --git a/generator/actions.ml b/generator/actions.ml > index fe492e6..850e58d 100644 > --- a/generator/actions.ml > +++ b/generator/actions.ml > @@ -10217,7 +10217,7 @@ See C<guestfs_get_e2generation>." }; > > { defaults with > name = "btrfs_subvolume_snapshot"; > - style = RErr, [Pathname "source"; Pathname "dest"], []; > + style = RErr, [Pathname "source"; Pathname "dest"], [OBool "ro"]; > proc_nr = Some 322; > optional = Some "btrfs"; camel_name = "BTRFSSubvolumeSnapshot"; > tests = [ You must set once_had_no_optargs = true for this action, as it currently has no optional arguments; see generator/README.