Libguestfs - Mar 2016 - [PATCH v2 3/3] fish: fix btrfs subvolumes display in error case

Hi all,

Here is version 2 of the patch series, including the changes for Pino's
remarks.

Cédric Bosdonnat (3):
  configure: handle older version of ncurses
  api: add mountable_device and mountable_subvolume
  fish: fix btrfs subvolumes display in error case

 fish/options.c          | 28 ++++++++++++++++++++++++++-
 generator/actions.ml    | 26 +++++++++++++++++++++++++
 m4/guestfs_libraries.m4 | 12 +++++++++++-
 po/POTFILES             |  1 +
 src/Makefile.am         |  1 +
 src/mountable.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 117 insertions(+), 2 deletions(-)
 create mode 100644 src/mountable.c

-- 
2.6.2

ncurses didn't have pkg-config files in not-that-old versions. If those
couldn't be found, then try the ncurses6-config tool.
---
 m4/guestfs_libraries.m4 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/m4/guestfs_libraries.m4 b/m4/guestfs_libraries.m4
index c5a4a01..defd394 100644
--- a/m4/guestfs_libraries.m4
+++ b/m4/guestfs_libraries.m4
@@ -113,7 +113,17 @@ struct sockaddr_un myaddr;
 dnl tgetent, tputs and UP [sic] are all required.  They come from the lower
 dnl tinfo library, but might be part of ncurses directly.
 PKG_CHECK_MODULES([LIBTINFO], [tinfo], [], [
-    PKG_CHECK_MODULES([LIBTINFO], [ncurses])
+    PKG_CHECK_MODULES([LIBTINFO], [ncurses], [], [
+        AC_CHECK_PROG([NCURSES_CONFIG], [ncurses6-config], [ncurses6-config],
[no])
+        if test "x$NCURSES_CONFIG" == "xno"; then
+            AC_CHECK_PROG([NCURSES_CONFIG], [ncurses5-config],
[ncurses5-config], [no])
+        fi
+        if test "x$NCURSES_CONFIG" == "xno"; then
+            AC_MSG_ERROR([ncurses development package is not installed])
+        fi
+        LIBTINFO_CFLAGS=`$NCURSES_CONFIG --cflags`
+        LIBTINFO_LIBS=`$NCURSES_CONFIG --libs`
+    ])
 ])
 AC_SUBST([LIBTINFO_CFLAGS])
 AC_SUBST([LIBTINFO_LIBS])
-- 
2.6.2

These two functions allow the user to split the mountable strings
into a device and a subvolume if any. See this thread on the mailing
list for the rationale:

https://www.redhat.com/archives/libguestfs/2016-February/msg00247.html
---
 generator/actions.ml | 26 ++++++++++++++++++++++++++
 po/POTFILES          |  1 +
 src/Makefile.am      |  1 +
 src/mountable.c      | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+)
 create mode 100644 src/mountable.c

diff --git a/generator/actions.ml b/generator/actions.ml
index 36d08ad..696fd87 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -1290,6 +1290,32 @@ Please read L<guestfs(3)/INSPECTION> for more
details.
 See also C<guestfs_inspect_get_mountpoints>." };
 
   { defaults with
+    name = "mountable_device"; added = (1, 33, 14);
+    style = RString "device", [Mountable "mountable"], [];
+    shortdesc = "extract the device part of a mountable";
+    longdesc = "\
+Returns the device name of a mountable. In quite a lot of
+cases, the mountable is the device name.
+
+However this doesn't apply for btrfs subvolumes, where the
+mountable is a combination of both the device name and the
+subvolume path (see also C<guestfs_mountable_subvolume> to
+extract the subvolume path of the mountable if any)." };
+
+  { defaults with
+    name = "mountable_subvolume"; added = (1, 33, 14);
+    style = RString "subvolume", [Mountable "mountable"],
[];
+    shortdesc = "extract the subvolume part of a mountable";
+    longdesc = "\
+Returns the subvolume path of a mountable. Btrfs subvolumes
+mountables are a combination of both the device name and the
+subvolume path (see also C<guestfs_mountable_device> to extract
+the device of the mountable).
+
+If the mountable does not represent a btrfs subvolume, then
+this function fails and the errno is set to EINVAL." };
+
+  { defaults with
     name = "set_network"; added = (1, 5, 4);
     style = RErr, [Bool "network"], [];
     fish_alias = ["network"]; config_only = true;
diff --git a/po/POTFILES b/po/POTFILES
index 4fbc551..195206f 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -346,6 +346,7 @@ src/libvirt-is-version.c
 src/listfs.c
 src/lpj.c
 src/match.c
+src/mountable.c
 src/osinfo.c
 src/private-data.c
 src/proto.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 60641bf..3b4cd10 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -121,6 +121,7 @@ libguestfs_la_SOURCES = \
 	listfs.c \
 	lpj.c \
 	match.c \
+	mountable.c \
 	osinfo.c \
 	private-data.c \
 	proto.c \
diff --git a/src/mountable.c b/src/mountable.c
new file mode 100644
index 0000000..12ba633
--- /dev/null
+++ b/src/mountable.c
@@ -0,0 +1,51 @@
+/* libguestfs
+ * Copyright (C) 2016 SUSE LLC
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+#include <string.h>
+#include <errno.h>
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+
+char *
+guestfs_impl_mountable_device (guestfs_h *g, const char *mountable)
+{
+    CLEANUP_FREE_INTERNAL_MOUNTABLE struct guestfs_internal_mountable *mnt =
NULL;
+
+    mnt = guestfs_internal_parse_mountable (g, mountable);
+    if (mnt == NULL)
+        return NULL;
+
+    return safe_strdup(g, mnt->im_device);
+}
+
+char *
+guestfs_impl_mountable_subvolume (guestfs_h *g, const char *mountable)
+{
+    CLEANUP_FREE_INTERNAL_MOUNTABLE struct guestfs_internal_mountable *mnt =
NULL;
+
+    mnt = guestfs_internal_parse_mountable (g, mountable);
+    if (mnt == NULL || STREQ (mnt->im_volume, "")) {
+        guestfs_int_error_errno (g, EINVAL, "not a btrfs subvolume
identifier");
+        return NULL;
+    }
+
+    return safe_strdup(g, mnt->im_volume);
+}
-- 
2.6.2

The list of filesystems that is printed when there was an error prints
the internal mountable string even for the btrfs subvolumes. Let's
printing a valid -m option value instead.
---
 fish/options.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/fish/options.c b/fish/options.c
index da5015d..9222b69 100644
--- a/fish/options.c
+++ b/fish/options.c
@@ -280,7 +280,33 @@ display_mountpoints_on_failure (const char *mp_device,
            guestfs_int_program_name);
 
   for (i = 0; fses[i] != NULL; i += 2) {
-    CLEANUP_FREE char *p = guestfs_canonical_device_name (g, fses[i]);
+    CLEANUP_FREE char *p = NULL;
+    CLEANUP_FREE char *device = guestfs_mountable_device(g, fses[i]);
+    CLEANUP_FREE char *subvolume = NULL;
+    int subvolume_errno;
+
+    guestfs_push_error_handler (g, NULL, NULL);
+
+    subvolume = guestfs_mountable_subvolume(g, fses[i]);
+    if (subvolume == NULL && guestfs_last_errno (g) != EINVAL) {
+      fprintf (stderr,
+               _("%s: cannot determine the subvolume for %s: %s
(%d)\n"),
+              guestfs_int_program_name, fses[i],
+              guestfs_last_error (g), guestfs_last_errno (g));
+    }
+
+    guestfs_pop_error_handler (g);
+
+    /* Reformat the internal btrfsvol string into a valid mount option */
+    if (device && subvolume) {
+      if (asprintf (&p, "%s:/:subvol=%s", device, subvolume) ==
-1) {
+          perror ("asprintf");
+          exit (EXIT_FAILURE);
+      }
+    } else {
+      p = guestfs_canonical_device_name (g, fses[i]);
+    }
+
     fprintf (stderr, "%s: \t%s (%s)\n", guestfs_int_program_name,
              p ? p : fses[i], fses[i+1]);
   }
-- 
2.6.2

On Tuesday 08 March 2016 10:26:57 Cédric Bosdonnat
wrote:
> ncurses didn't have pkg-config files in not-that-old versions. If those
> couldn't be found, then try the ncurses6-config tool.
> ---
>  m4/guestfs_libraries.m4 | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/guestfs_libraries.m4 b/m4/guestfs_libraries.m4
> index c5a4a01..defd394 100644
> --- a/m4/guestfs_libraries.m4
> +++ b/m4/guestfs_libraries.m4
> @@ -113,7 +113,17 @@ struct sockaddr_un myaddr;
>  dnl tgetent, tputs and UP [sic] are all required.  They come from the
lower
>  dnl tinfo library, but might be part of ncurses directly.
>  PKG_CHECK_MODULES([LIBTINFO], [tinfo], [], [
> -    PKG_CHECK_MODULES([LIBTINFO], [ncurses])
> +    PKG_CHECK_MODULES([LIBTINFO], [ncurses], [], [
> +        AC_CHECK_PROG([NCURSES_CONFIG], [ncurses6-config],
[ncurses6-config], [no])
> +        if test "x$NCURSES_CONFIG" == "xno"; then
> +            AC_CHECK_PROG([NCURSES_CONFIG], [ncurses5-config],
[ncurses5-config], [no])
> +        fi
I think this is what AC_CHECK_PROGS does, supplying more executables
to search.
> +        if test "x$NCURSES_CONFIG" == "xno"; then
> +            AC_MSG_ERROR([ncurses development package is not installed])
> +        fi
> +        LIBTINFO_CFLAGS=`$NCURSES_CONFIG --cflags`
> +        LIBTINFO_LIBS=`$NCURSES_CONFIG --libs`
> +    ])
>  ])
>  AC_SUBST([LIBTINFO_CFLAGS])
>  AC_SUBST([LIBTINFO_LIBS])
With the above change, it looks ok to me.

Thanks,
-- 
Pino Toscano

On Tuesday 08 March 2016 10:26:58 Cédric Bosdonnat
wrote:
> These two functions allow the user to split the mountable strings
> into a device and a subvolume if any. See this thread on the mailing
> list for the rationale:
> 
> https://www.redhat.com/archives/libguestfs/2016-February/msg00247.html
> ---
>  generator/actions.ml | 26 ++++++++++++++++++++++++++
>  po/POTFILES          |  1 +
>  src/Makefile.am      |  1 +
>  src/mountable.c      | 51
+++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 79 insertions(+)
>  create mode 100644 src/mountable.c
> 
> diff --git a/generator/actions.ml b/generator/actions.ml
> index 36d08ad..696fd87 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -1290,6 +1290,32 @@ Please read L<guestfs(3)/INSPECTION> for more
details.
>  See also C<guestfs_inspect_get_mountpoints>." };
>  
>    { defaults with
> +    name = "mountable_device"; added = (1, 33, 14);
Next version is 1.33.15 now.
> +    style = RString "device", [Mountable "mountable"],
[];
> +    shortdesc = "extract the device part of a mountable";
> +    longdesc = "\
> +Returns the device name of a mountable. In quite a lot of
> +cases, the mountable is the device name.
> +
> +However this doesn't apply for btrfs subvolumes, where the
> +mountable is a combination of both the device name and the
> +subvolume path (see also C<guestfs_mountable_subvolume> to
> +extract the subvolume path of the mountable if any)." };
> +
> +  { defaults with
> +    name = "mountable_subvolume"; added = (1, 33, 14);
> +    style = RString "subvolume", [Mountable
"mountable"], [];
> +    shortdesc = "extract the subvolume part of a mountable";
> +    longdesc = "\
> +Returns the subvolume path of a mountable. Btrfs subvolumes
> +mountables are a combination of both the device name and the
> +subvolume path (see also C<guestfs_mountable_device> to extract
> +the device of the mountable).
> +
> +If the mountable does not represent a btrfs subvolume, then
> +this function fails and the errno is set to EINVAL." };
> +
> +  { defaults with
>      name = "set_network"; added = (1, 5, 4);
>      style = RErr, [Bool "network"], [];
>      fish_alias = ["network"]; config_only = true;
> diff --git a/po/POTFILES b/po/POTFILES
> index 4fbc551..195206f 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -346,6 +346,7 @@ src/libvirt-is-version.c
>  src/listfs.c
>  src/lpj.c
>  src/match.c
> +src/mountable.c
>  src/osinfo.c
>  src/private-data.c
>  src/proto.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 60641bf..3b4cd10 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -121,6 +121,7 @@ libguestfs_la_SOURCES = \
>  	listfs.c \
>  	lpj.c \
>  	match.c \
> +	mountable.c \
>  	osinfo.c \
>  	private-data.c \
>  	proto.c \
> diff --git a/src/mountable.c b/src/mountable.c
> new file mode 100644
> index 0000000..12ba633
> --- /dev/null
> +++ b/src/mountable.c
> @@ -0,0 +1,51 @@
> +/* libguestfs
> + * Copyright (C) 2016 SUSE LLC
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA
> + */
> +
> +#include <config.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include "guestfs.h"
> +#include "guestfs-internal.h"
> +
> +
> +char *
> +guestfs_impl_mountable_device (guestfs_h *g, const char *mountable)
> +{
> +    CLEANUP_FREE_INTERNAL_MOUNTABLE struct guestfs_internal_mountable *mnt
= NULL;
> +
> +    mnt = guestfs_internal_parse_mountable (g, mountable);
> +    if (mnt == NULL)
> +        return NULL;
Please note the indentation is 2 spaces ...
> +    return safe_strdup(g, mnt->im_device);
... and add a space before round brackets (both apply to the other
function as well).
> +char *
> +guestfs_impl_mountable_subvolume (guestfs_h *g, const char *mountable)
> +{
> +    CLEANUP_FREE_INTERNAL_MOUNTABLE struct guestfs_internal_mountable *mnt
= NULL;
> +
> +    mnt = guestfs_internal_parse_mountable (g, mountable);
> +    if (mnt == NULL || STREQ (mnt->im_volume, "")) {
> +        guestfs_int_error_errno (g, EINVAL, "not a btrfs subvolume
identifier");
> +        return NULL;
> +    }
> +
> +    return safe_strdup(g, mnt->im_volume);
> +}
> 
Except from the notes above, the new APIs look fine for me -- Rich?

Thanks,
-- 
Pino Toscano

On Tuesday 08 March 2016 10:26:59 Cédric Bosdonnat
wrote:
> The list of filesystems that is printed when there was an error prints
> the internal mountable string even for the btrfs subvolumes. Let's
> printing a valid -m option value instead.
> ---
>  fish/options.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/fish/options.c b/fish/options.c
> index da5015d..9222b69 100644
> --- a/fish/options.c
> +++ b/fish/options.c
> @@ -280,7 +280,33 @@ display_mountpoints_on_failure (const char *mp_device,
>             guestfs_int_program_name);
>  
>    for (i = 0; fses[i] != NULL; i += 2) {
> -    CLEANUP_FREE char *p = guestfs_canonical_device_name (g, fses[i]);
> +    CLEANUP_FREE char *p = NULL;
> +    CLEANUP_FREE char *device = guestfs_mountable_device(g, fses[i]);
Same small coding style issues (missing space before round bracket,
indentation) also in few lines of this patch.
> +    CLEANUP_FREE char *subvolume = NULL;
> +    int subvolume_errno;
This variable looks unused.
> +
> +    guestfs_push_error_handler (g, NULL, NULL);
> +
> +    subvolume = guestfs_mountable_subvolume(g, fses[i]);
> +    if (subvolume == NULL && guestfs_last_errno (g) != EINVAL) {
> +      fprintf (stderr,
> +               _("%s: cannot determine the subvolume for %s: %s
(%d)\n"),
> +              guestfs_int_program_name, fses[i],
> +              guestfs_last_error (g), guestfs_last_errno (g));
Most probably exit (EXIT_FAILURE) is needed here, as it represents a
failure.
> +    }
> +
> +    guestfs_pop_error_handler (g);
> +
> +    /* Reformat the internal btrfsvol string into a valid mount option */
> +    if (device && subvolume) {
> +      if (asprintf (&p, "%s:/:subvol=%s", device, subvolume)
== -1) {
> +          perror ("asprintf");
> +          exit (EXIT_FAILURE);
> +      }
> +    } else {
> +      p = guestfs_canonical_device_name (g, fses[i]);
> +    }
> +
>      fprintf (stderr, "%s: \t%s (%s)\n",
guestfs_int_program_name,
>               p ? p : fses[i], fses[i+1]);
>    }
Otherwise, the patch seems fine for me.

Thanks,
-- 
Pino Toscano

On Tue, Mar 08, 2016 at 10:26:58AM +0100, Cédric Bosdonnat
wrote:
> +If the mountable does not represent a btrfs subvolume, then
> +this function fails and the errno is set to EINVAL." };
C<errno> and C<EINVAL> are probably better here, as it will cause
the
words to be marked up as code.

Rest is OK with Pino's suggested changes.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
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