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