Libguestfs - Mar 2016 - [PATCH v3 0/3] btrfs subvolumes display fix

Hi there,

Latest and greatest version including all new remarks from both Pino and Rich.

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 |  9 ++++++++-
 po/POTFILES             |  1 +
 src/Makefile.am         |  1 +
 src/mountable.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 114 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 and ncurses5-config tools.
---
 m4/guestfs_libraries.m4 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/m4/guestfs_libraries.m4 b/m4/guestfs_libraries.m4
index c5a4a01..e845ea9 100644
--- a/m4/guestfs_libraries.m4
+++ b/m4/guestfs_libraries.m4
@@ -113,7 +113,14 @@ 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_PROGS([NCURSES_CONFIG], [ncurses6-config ncurses5-config],
[no])
+        AS_IF([test "x$NCURSES_CONFIG" = "xno"], [
+            AC_MSG_ERROR([ncurses development package is not installed])
+        ])
+        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..51f2295 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, 15);
+    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, 15);
+    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 C<errno> is set to C<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..fb982df
--- /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..0dffff5 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;
+
+    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));
+      exit (EXIT_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]);
   }
-- 
2.6.2

On Tuesday 08 March 2016 16:04:21 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..51f2295 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, 15);
> +    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, 15);
> +    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 C<errno> is set to
C<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..fb982df
> --- /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"
> +
I'm noticing now that the guestfs-internal-actions.h include is missing
here, and thus the functions here will not build because of missing
prototypes.

I'm fixing manually and pushing, no need to send a new version of this
patch now.  My suggestion is to configure libguestfs for development
with --enable-werror, so warnings are turned into errors (and thus are
harder to miss).

Thanks,
-- 
Pino Toscano

On Tuesday 08 March 2016 16:04:19 Cédric Bosdonnat
wrote:
> Latest and greatest version including all new remarks from both Pino and
Rich.
> 
> 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 |  9 ++++++++-
>  po/POTFILES             |  1 +
>  src/Makefile.am         |  1 +
>  src/mountable.c         | 51
+++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 114 insertions(+), 2 deletions(-)
>  create mode 100644 src/mountable.c
Just with a small amend in patch #2, series tested fine and pushed.

Thanks,
-- 
Pino Toscano