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

Hey there!

Here are a few patches to fix unrelated things: one fixes the configure for
older
ncurses releases having no pkg-config files. The other two are fixing what
Richard
mentioned about guestfs subvolumes display


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          | 15 ++++++++++++++-
 generator/actions.ml    | 25 +++++++++++++++++++++++++
 m4/guestfs_libraries.m4 |  6 +++++-
 po/POTFILES             |  1 +
 src/Makefile.am         |  1 +
 src/mountable.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 94 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/m4/guestfs_libraries.m4 b/m4/guestfs_libraries.m4
index c5a4a01..0aadae0 100644
--- a/m4/guestfs_libraries.m4
+++ b/m4/guestfs_libraries.m4
@@ -113,7 +113,11 @@ 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([NCURSES6_CONFIG], [ncurses6-config], [ncurses6-config],
[no])
+        LIBTINFO_CFLAGS=`$NCURSES6_CONFIG --cflags`
+        LIBTINFO_LIBS=`$NCURSES6_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 | 25 +++++++++++++++++++++++++
 po/POTFILES          |  1 +
 src/Makefile.am      |  1 +
 src/mountable.c      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+)
 create mode 100644 src/mountable.c

diff --git a/generator/actions.ml b/generator/actions.ml
index eb45392..454164f 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -1290,6 +1290,31 @@ Please read L<guestfs(3)/INSPECTION> for more
details.
 See also C<guestfs_inspect_get_mountpoints>." };
 
   { defaults with
+    name = "mountable_device"; added = (1, 33, 13);
+    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, 13);
+    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 has no subvolume part, then the errno (see
+C<guestfs_last_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 0fb99b0..4912f9f 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -345,6 +345,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..8f27606
--- /dev/null
+++ b/src/mountable.c
@@ -0,0 +1,48 @@
+/* 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 <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;
+
+    if ((mnt = guestfs_internal_parse_mountable (g, mountable)))
+        return safe_strdup(g, mnt->im_device);
+
+    return NULL;
+}
+
+char *
+guestfs_impl_mountable_subvolume (guestfs_h *g, const char *mountable)
+{
+    CLEANUP_FREE_INTERNAL_MOUNTABLE struct guestfs_internal_mountable *mnt =
NULL;
+
+    if ((mnt = guestfs_internal_parse_mountable (g, mountable)))
+        return safe_strdup(g, mnt->im_volume);
+
+    errno = EINVAL;
+    return NULL;
+}
-- 
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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fish/options.c b/fish/options.c
index da5015d..5c09bfb 100644
--- a/fish/options.c
+++ b/fish/options.c
@@ -280,7 +280,20 @@ 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 *volume = guestfs_mountable_subvolume(g, fses[i]);
+
+    /* Reformat the internal btrfsvol string into a valid mount option */
+    if (device && volume && guestfs_last_errno (g) != EINVAL) {
+      if (asprintf (&p, "%s:/:subvol=%s", device, volume) == -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 01 March 2016 11:17:17 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 | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/guestfs_libraries.m4 b/m4/guestfs_libraries.m4
> index c5a4a01..0aadae0 100644
> --- a/m4/guestfs_libraries.m4
> +++ b/m4/guestfs_libraries.m4
> @@ -113,7 +113,11 @@ 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([NCURSES6_CONFIG], [ncurses6-config],
[ncurses6-config], [no])
> +        LIBTINFO_CFLAGS=`$NCURSES6_CONFIG --cflags`
> +        LIBTINFO_LIBS=`$NCURSES6_CONFIG --libs`
AC_CHECK_PROG won't fail if the program is not found, so instead of
setting the "not found" value to "no" (which will cause the
two
invocations to fail because "no" is not found), use AC_MSG_ERROR to
clearly point out about the issue.

Also, I guess you should check both ncurses6-config and
ncurses5-config?

Thanks,
-- 
Pino Toscano

On Tuesday 01 March 2016 11:17:18 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 | 25 +++++++++++++++++++++++++
>  po/POTFILES          |  1 +
>  src/Makefile.am      |  1 +
>  src/mountable.c      | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 75 insertions(+)
>  create mode 100644 src/mountable.c
> 
> diff --git a/generator/actions.ml b/generator/actions.ml
> index eb45392..454164f 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -1290,6 +1290,31 @@ Please read L<guestfs(3)/INSPECTION> for more
details.
>  See also C<guestfs_inspect_get_mountpoints>." };
>  
>    { defaults with
> +    name = "mountable_device"; added = (1, 33, 13);
1.33.13 is tagged now, so this should be 1.33.14; it is not a big deal
though, as it will be adjusted on the fly when applying the commit by
whoever (Rich or me) is pushing it.
> +    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)." };
I'd split the longdesc in two paragraph, the first describing what the
API does, and the second about the case with btrfs.

I'm not a native English speaker though.
> +  { defaults with
> +    name = "mountable_subvolume"; added = (1, 33, 13);
> +    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 has no subvolume part, then the errno (see
> +C<guestfs_last_errno>) is set to C<EINVAL>." };
I'd say something like "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 0fb99b0..4912f9f 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -345,6 +345,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..8f27606
> --- /dev/null
> +++ b/src/mountable.c
> @@ -0,0 +1,48 @@
> +/* 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 <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;
> +
> +    if ((mnt = guestfs_internal_parse_mountable (g, mountable)))
> +        return safe_strdup(g, mnt->im_device);
Avoid side-effects in if statements, and fail earlier:

  mnt = guestfs_internal_parse_mountable (g, mountable);
  if (mnt == NULL)
    return NULL;

  return safe_strdup (g, mnt->im_device);

> +
> +    return NULL;
> +}
> +
> +char *
> +guestfs_impl_mountable_subvolume (guestfs_h *g, const char *mountable)
> +{
> +    CLEANUP_FREE_INTERNAL_MOUNTABLE struct guestfs_internal_mountable *mnt
= NULL;
> +
> +    if ((mnt = guestfs_internal_parse_mountable (g, mountable)))
> +        return safe_strdup(g, mnt->im_volume);
Ditto.
> +    errno = EINVAL;
libguestfs' errno is not the system one -- there's a proper internal
API for this, see the various error/warning functions/macros in
src/guestfs-internal.h.  In this case, you need to use
guestfs_int_error_errno explicitly:

   guestfs_int_error_errno (g, EINVAL, "not a btrfs subvolume
identifier");

Also, note that in case of non-btrfs mountables,
guestfs_internal_parse_mountable succeeds but im_volume will be an
empty string, so you need to consider that.

Thanks,
-- 
Pino Toscano

On Tuesday 01 March 2016 11:17:19 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 | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/fish/options.c b/fish/options.c
> index da5015d..5c09bfb 100644
> --- a/fish/options.c
> +++ b/fish/options.c
> @@ -280,7 +280,20 @@ 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 *volume = guestfs_mountable_subvolume(g, fses[i]);
If guestfs_mountable_subvolume fails, then an error is issued to the
error handler set: you need to push a temporary null error handler to
avoid that:

  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));
    exit (EXIT_FAILURE);
  }
  guestfs_pop_error_handler (g);
> +    /* Reformat the internal btrfsvol string into a valid mount option */
> +    if (device && volume && guestfs_last_errno (g) !=
EINVAL) {
With the above, this needs to check device and subvolume only.
> +      if (asprintf (&p, "%s:/:subvol=%s", device, volume) ==
-1) {
> +          perror ("asprintf");
> +          exit (EXIT_FAILURE);
> +      }
> +    } else {
> +      p = guestfs_canonical_device_name (g, fses[i]);
> +    }
Usually there are no curly brackets for blocks of a single instruction.

Thanks,
-- 
Pino Toscano