[Libguestfs] [PATCH 2/3] api: add mountable_device and mountable_subvolume

[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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160301/408cee74/attachment.sig>