Re: [PATCH 1/5] Add support for read-only subvolumes.

[Chris Mason <chris.mason@oracle.com>]

Excerpts from Goffredo Baroncelli's message of 2011-04-25 17:34:46 -0400:
> Hi Andreas,
> 
> On 04/25/2011 03:47 PM, Andreas Philipp wrote:
> > Use BTRFS_IOC_CREATE_SNAP_V2 instead of BTRFS_IOC_CREATE_SNAP and add
> > an option for the creation of a readonly snapshot.
> > 
> > Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
> > ---
> >  btrfs_cmds.c |   44 ++++++++++++++++++++++++++++++++++++--------
> >  1 files changed, 36 insertions(+), 8 deletions(-)
> > 
> > diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> > index 8031c58..baec675 100644
> > --- a/btrfs_cmds.c
> > +++ b/btrfs_cmds.c
> > @@ -43,7 +43,7 @@
> >  
> >  #ifdef __CHECKER__
> 
> It is not related to your parch.. but does anyone know why we re-define
> some constants if __CHECKER__ is defined ?

This is old support for the sparse program, which finds a good number of
bugs.

> 
> >  #define BLKGETSIZE64 0
> > -#define BTRFS_IOC_SNAP_CREATE 0
> > +#define BTRFS_IOC_SNAP_CREATE_V2 0
> >  #define BTRFS_VOL_NAME_MAX 255
> >  struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
> >  static inline int ioctl(int fd, int define, void *arg) { return 0; }
> > @@ -310,13 +310,34 @@ int do_subvol_list(int argc, char **argv)
> >  int do_clone(int argc, char **argv)
> >  {
> >      char    *subvol, *dst;
> > -    int    res, fd, fddst, len;
> > +    int    res, fd, fddst, len, optind = 0, readonly = 0;
> >      char    *newname;
> >      char    *dstdir;
> >  
> > -    subvol = argv[1];
> > -    dst = argv[2];
> > -    struct btrfs_ioctl_vol_args    args;
> > +    while(1) {
> > +        int c = getopt(argc, argv, "r");
> > +        if (c < 0)
> > +            break;
> > +        switch(c) {
> > +            case 'r':
> > +                optind++;
> > +                readonly = 1;
> > +                break;
> > +                    default:
> > +                fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> > +                free(argv);
> > +                return 1;
> > +        }
> > +    }
> > +    if (argc - optind < 2) {
> > +        fprintf(stderr, "Invalid arguments for defragment\n");
>                                                        ^^^^^^^^^^^^
> 
> May be that you need to replace "defragment" with "subvolume snapshot" ?
> 
> > +        free(argv);
> > +        return 1;
> > +    }
> > +
> > +    subvol = argv[optind+1];
> > +    dst = argv[optind+2];
> > +    struct btrfs_ioctl_vol_args_v2    args;
> 
> Does the "standard C" allow to define a variable in the middle in a
> function instead of in the begin ?
> Anyway, even not required, I suggest to fill "args" by zero.

I'd prefer all the variables are at the stop of a scope.

> 
> +    memset(&args, 0, sizeog(args));
> 
> >  
> >      res = test_issubvolume(subvol);
> >      if(res<0){
> > @@ -371,12 +392,19 @@ int do_clone(int argc, char **argv)
> >          fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
> >          return 12;
> >      }
> > +    
> > +    if (readonly) {
> > +        args.flags |= BTRFS_SUBVOL_RDONLY;
> > +        printf("Create a readonly snapshot of '%s' in '%s/%s'\n",
> > +               subvol, dstdir, newname);
> > +    }
> > +    else
> > +        printf("Create a snapshot of '%s' in '%s/%s'\n",
> > +               subvol, dstdir, newname);
> >  
> It is not required, but I suggest to use also in the "else" the brackets
> ( {} ).

Yes please.

> 
> > -    printf("Create a snapshot of '%s' in '%s/%s'\n",
> > -           subvol, dstdir, newname);
> >      args.fd = fd;
> >      strcpy(args.name, newname);
> > -    res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args);
> > +    res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
> >  
> >      close(fd);
> >      close(fddst);
> 

-chris