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

[Hu Tao]

On Fri, Nov 21, 2014 at 11:38:54AM +0100, Pino Toscano wrote:
> 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 at cn.fujitsu.com>
> > ---
> >  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.

Okay.

> 
> 
> > 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.

Thanks for catching this!

Regards,
Hu