Libguestfs - Jul 2016 - Re: [PATCH 1/3] mllib: add checking for btrfs subvolume

On Thu, Jul 07, 2016 at 06:04:14PM +0300, Maxim Perevedentsev
wrote:
> This is needed to skip btrfs subvolumes from output
> of list_filesystems where device is needed.
> ---
>  mllib/common_utils.ml  | 10 ++++++++++
>  mllib/common_utils.mli |  3 +++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> index 77b9acd..30fc5cd 100644
> --- a/mllib/common_utils.ml
> +++ b/mllib/common_utils.ml
> @@ -922,3 +922,13 @@ let inspect_mount_root g ?mount_opts_fn root >  
>  let inspect_mount_root_ro >    inspect_mount_root ~mount_opts_fn:(fun _
-> "ro")
> +
> +let is_btrfs_subvolume g fs > +  let device = g#mountable_device fs in
> +  let subvol > +    try
> +      g#mountable_subvolume fs
> +    with Guestfs.Error msg as exn ->
> +      if g#last_errno () = Guestfs.Errno.errno_EINVAL then ""
> +      else raise exn in
> +  device <> "" && subvol <> ""
Firstly I think device <> "" is always true.

Secondly it wasn't obvious to me until I thought about it that you're
testing if subvol is not equal to the value ("") returned three lines
earlier.

How about this, which is type safe and a lot simpler:

let is_btrfs_subvolume g fs   let device = g#mountable_device fs in
  try
    ignore (g#mountable_subvolume fs); true
  with Guestfs.Error msg as exn ->
    if g#last_errno () = Guestfs.Errno.errno_EINVAL then false
    else raise exn

Apart from that, this is just a translation of fish/options.c:
display_mountpoints_on_failure into OCaml, so ACK if that was fixed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Thursday 07 July 2016 16:54:20 Richard W.M. Jones
wrote:
> On Thu, Jul 07, 2016 at 06:04:14PM +0300, Maxim Perevedentsev wrote:
> > This is needed to skip btrfs subvolumes from output
> > of list_filesystems where device is needed.
> > ---
> >  mllib/common_utils.ml  | 10 ++++++++++
> >  mllib/common_utils.mli |  3 +++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/mllib/common_utils.ml b/mllib/common_utils.ml
> > index 77b9acd..30fc5cd 100644
> > --- a/mllib/common_utils.ml
> > +++ b/mllib/common_utils.ml
> > @@ -922,3 +922,13 @@ let inspect_mount_root g ?mount_opts_fn root >
>
> >  let inspect_mount_root_ro > >    inspect_mount_root
~mount_opts_fn:(fun _ -> "ro")
> > +
> > +let is_btrfs_subvolume g fs > > +  let device =
g#mountable_device fs in
> > +  let subvol > > +    try
> > +      g#mountable_subvolume fs
> > +    with Guestfs.Error msg as exn ->
> > +      if g#last_errno () = Guestfs.Errno.errno_EINVAL then
""
> > +      else raise exn in
> > +  device <> "" && subvol <>
""
> 
> Firstly I think device <> "" is always true.
> 
> Secondly it wasn't obvious to me until I thought about it that
you're
> testing if subvol is not equal to the value ("") returned three
lines
> earlier.
> 
> How about this, which is type safe and a lot simpler:
> 
> let is_btrfs_subvolume g fs >   let device = g#mountable_device fs in
>   try
>     ignore (g#mountable_subvolume fs); true
>   with Guestfs.Error msg as exn ->
>     if g#last_errno () = Guestfs.Errno.errno_EINVAL then false
>     else raise exn
This is what I was writing too, but you were faster :)
I think this will fail to build as "device" is unused, so the first
line should be:

  (* Make sure we can determine the device of the mountable. *)
  ignore (g#mountable_device fs);
> Apart from that, this is just a translation of fish/options.c:
> display_mountpoints_on_failure into OCaml, so ACK if that was fixed.
Indeed, LGTM with the above fix.

Thanks,
-- 
Pino Toscano

On Thu, Jul 07, 2016 at 06:04:04PM +0200, Pino Toscano
wrote:
> On Thursday 07 July 2016 16:54:20 Richard W.M. Jones wrote:
> > How about this, which is type safe and a lot simpler:
> > 
> > let is_btrfs_subvolume g fs > >   let device =
g#mountable_device fs in
> >   try
> >     ignore (g#mountable_subvolume fs); true
> >   with Guestfs.Error msg as exn ->
> >     if g#last_errno () = Guestfs.Errno.errno_EINVAL then false
> >     else raise exn
> 
> This is what I was writing too, but you were faster :)
> I think this will fail to build as "device" is unused, so the
first
> line should be:
> 
>   (* Make sure we can determine the device of the mountable. *)
>   ignore (g#mountable_device fs);
Actually I believe Maxim can just drop the 'let device = ...' line
completely.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html