[Libguestfs] [PATCH 4/7] New internal API: internal_parse_mountable
Richard W.M. Jones
rjones at redhat.com
Tue Feb 12 12:21:37 UTC 2013
On Tue, Feb 12, 2013 at 11:04:23AM +0000, Matthew Booth wrote:
> This API allows the library to parse a mountable string.
>
> This change also moves the mountable_type_t enum into mountable.h in
> the library, and includes it automatically for code defining
> GUESTFS_PRIVATE. As mountable.h is not installed, this will mean that
> GUESTFS_PRIVATE will not be usable outside a build tree.
My understanding of "mountable.h" is that it defines the
mountable_type_t enum. Since this is shared between the library and
the daemon, it should go into the existing file for this purpose
"guestfs-internal-all.h", get rid of "mountable.h", and get rid of the
include from <guestfs.h>.
> .gitignore | 2 +
> Makefile.am | 1 +
> configure.ac | 1 +
> daemon/Makefile.am | 21 ++--
> daemon/daemon.h | 6 +-
> daemon/mountable.c | 60 +++++++++
> generator/actions.ml | 9 ++
> generator/c.ml | 2 +
> generator/structs.ml | 10 ++
> po/POTFILES | 1 +
> src/MAX_PROC_NR | 2 +-
> src/mountable.h | 36 ++++++
> tests/mountable/Makefile.am | 38 ++++++
> tests/mountable/test-internal_parse_mountable.c | 156 ++++++++++++++++++++++++
> 14 files changed, 328 insertions(+), 17 deletions(-)
> create mode 100644 daemon/mountable.c
> create mode 100644 src/mountable.h
> create mode 100644 tests/mountable/Makefile.am
> create mode 100644 tests/mountable/test-internal_parse_mountable.c
>
> diff --git a/.gitignore b/.gitignore
> index 8b88abd..5e543e9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -76,6 +76,7 @@ Makefile.in
> /daemon/guestfs_protocol.h
> /daemon/install-sh
> /daemon/missing
> +/daemon/mountable.h
> /daemon/names.c
> /daemon/optgroups.c
> /daemon/optgroups.h
> @@ -436,6 +437,7 @@ Makefile.in
> /tests/guests/ubuntu.img
> /tests/guests/windows.img
> /tests/mount-local/test-parallel-mount-local
> +/tests/mountable/test-internal_parse_mountable
I've been trying to use '-' instead of '_' to separate parts of
filenames, except in a few cases where this is literally not possible
(currently: guestfs_protocol.x because of a shortcoming in rpcgen, and
some OCaml files because _ is required in OCaml module names, and
there are some C example programs that I'm going to change).
> /tests/parallel/test-parallel
> /tests/regressions/rhbz501893
> /tests/regressions/rhbz790721
> diff --git a/Makefile.am b/Makefile.am
> index 842008a..73a46f8 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -36,6 +36,7 @@ endif
>
> # Tests - order is important.
> if ENABLE_APPLIANCE
> +SUBDIRS += tests/mountable
> SUBDIRS += tests/qemu
> SUBDIRS += tests/guests
> SUBDIRS += tests/c-api
> diff --git a/configure.ac b/configure.ac
> index be713f6..7ee04a8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1546,6 +1546,7 @@ AC_CONFIG_FILES([Makefile
> tests/lvm/Makefile
> tests/md/Makefile
> tests/mount-local/Makefile
> + tests/mountable/Makefile
> tests/ntfsclone/Makefile
> tests/parallel/Makefile
> tests/protocol/Makefile
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index a1e5e68..fa5343a 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -26,26 +26,23 @@ generator_built = \
> stubs.c \
> names.c
>
> -BUILT_SOURCES = \
> - $(generator_built) \
> +shared_with_library = \
> guestfs_protocol.c \
> guestfs_protocol.h \
> - errnostring-gperf.c \
> errnostring-gperf.gperf \
> errnostring.c \
> - errnostring.h
> + errnostring.h \
> + mountable.h
> +
> +BUILT_SOURCES = \
> + $(generator_built) \
> + $(shared_with_library) \
> + errnostring-gperf.c
>
> EXTRA_DIST = \
> $(BUILT_SOURCES) \
> guestfsd.pod
>
> -shared_with_library = \
> - guestfs_protocol.c \
> - guestfs_protocol.h \
> - errnostring-gperf.gperf \
> - errnostring.c \
> - errnostring.h
> -
> $(shared_with_library): %: $(libsrcdir)/%
> rm -f $@
> ln $< $@
> @@ -144,6 +141,8 @@ guestfsd_SOURCES = \
> mktemp.c \
> modprobe.c \
> mount.c \
> + mountable.c \
> + mountable.h \
> names.c \
> ntfs.c \
> ntfsclone.c \
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index d343dfd..bb11fe9 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -34,11 +34,7 @@
>
> /* Mountables */
>
> -typedef enum {
> - MOUNTABLE_DEVICE, /* A bare device */
> - MOUNTABLE_BTRFSVOL, /* A btrfs subvolume: device + volume */
> - MOUNTABLE_PATH /* An already mounted path: device = path */
> -} mountable_type_t;
> +#include "mountable.h"
>
> typedef struct {
> mountable_type_t type;
> diff --git a/daemon/mountable.c b/daemon/mountable.c
> new file mode 100644
> index 0000000..170e579
> --- /dev/null
> +++ b/daemon/mountable.c
> @@ -0,0 +1,60 @@
> +/* libguestfs - the guestfsd daemon
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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 General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "daemon.h"
> +#include "actions.h"
> +#include "guestfs_protocol.h"
> +
> +guestfs_int_mountable_internal *
> +do_internal_parse_mountable (const mountable_t *mountable)
> +{
> + guestfs_int_mountable_internal *ret = calloc (1, sizeof *ret);
> + if (ret == NULL) {
> + reply_with_perror ("calloc");
> + return NULL;
> + }
> +
> + ret->type = mountable->type;
> + if (mountable->device) {
> + ret->device = strdup (mountable->device);
> + if (!ret->device) {
> + reply_with_perror ("strdup");
> + free (ret);
> + return NULL;
> + }
> + }
> +
> + if (mountable->volume) {
> + ret->volume = strdup (mountable->volume);
> + if (!ret->volume) {
> + reply_with_perror ("strdup");
> + free (ret->device);
> + free (ret);
> + return NULL;
> + }
> + }
> +
> + fprintf (stderr, "!!!!!!=======!!!!!!!! %i %s %s\n", ret->type, ret->device, ret->volume);
Is this left over debugging?
It's OK to print stuff, but in the final code it should be defended by
'if (verbose)' (in the daemon), or should call 'debug' (in the
library).
> + return ret;
> +}
> diff --git a/generator/actions.ml b/generator/actions.ml
> index f17cb6a..cd6f9c4 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -10726,6 +10726,15 @@ you are better to use C<guestfs_mv> instead." };
> This returns C<true> if and only if C<device> refers to a whole block
> device. That is, not a partition or a logical device." };
>
> + { defaults with
> + name = "internal_parse_mountable";
> + style = RStruct ("ret", "mountable_internal"), [Mountable "mountable"], [];
As I think discussed previously, the struct should be called
"internal_mountable". Don't use "ret" as the name of the return value
(the generator should probably prevent this), call it something like
"mountable".
> + visibility = VInternal;
> + proc_nr = Some 396;
> + shortdesc = "parse a mountable string";
> + longdesc = "\
> +Parse a mountable string." };
> +
> ]
>
> (* Non-API meta-commands available only in guestfish.
> diff --git a/generator/c.ml b/generator/c.ml
> index e2ecb8a..8e8033b 100644
> --- a/generator/c.ml
> +++ b/generator/c.ml
> @@ -709,6 +709,8 @@ extern GUESTFS_DLL_PUBLIC void *guestfs_next_private (guestfs_h *g, const char *
> * they are used by some of the language bindings.
> */
>
> +#include \"mountable.h\"
> +
> /* Private functions. */
>
> extern GUESTFS_DLL_PUBLIC int guestfs___for_each_disk (guestfs_h *g, /* virDomainPtr */ void *dom, int (*)(guestfs_h *g, const char *filename, const char *format, int readonly, void *data), void *data);
> diff --git a/generator/structs.ml b/generator/structs.ml
> index 82d5b7f..353d83b 100644
> --- a/generator/structs.ml
> +++ b/generator/structs.ml
> @@ -360,6 +360,16 @@ let structs = [
> "hivex_value_h", FInt64;
> ];
> s_camel_name = "HivexValue" };
> + { defaults with
> + s_name = "mountable_internal";
> + s_internal = true;
> + s_cols = [
> + "type", FInt32;
> + "device", FString;
> + "volume", FString;
I'm slightly surprised this code compiles. Anyway for the benefit of
OCaml you'll want to call those fields "mi_type", "mi_device" and
"mi_volume".
> + ];
> + s_camel_name = "MountableInternal";
> + };
> ] (* end of structs *)
>
> let lookup_struct name =
> diff --git a/po/POTFILES b/po/POTFILES
> index 7ceb594..0686966 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -64,6 +64,7 @@ daemon/mknod.c
> daemon/mktemp.c
> daemon/modprobe.c
> daemon/mount.c
> +daemon/mountable.c
> daemon/names.c
> daemon/ntfs.c
> daemon/ntfsclone.c
> diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> index e537bfe..4391a33 100644
> --- a/src/MAX_PROC_NR
> +++ b/src/MAX_PROC_NR
> @@ -1 +1 @@
> -395
> +396
> diff --git a/src/mountable.h b/src/mountable.h
> new file mode 100644
> index 0000000..f95f3c9
> --- /dev/null
> +++ b/src/mountable.h
> @@ -0,0 +1,36 @@
> +/* libguestfs
> + *
> + * Copyright (C) 2013 Red Hat Inc.
> + *
> + * 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
> + */
> +
> +
> +#ifndef GUESTFS_MOUNTABLE_H_
> +#define GUESTFS_MOUNTABLE_H_
> +
> +/* The type field of a parsed mountable.
> + *
> + * This is used both by mountable_t in the daemon, and
> + * struct guestfs_int_mountable_internal in the library.
> + */
> +
> +typedef enum {
> + MOUNTABLE_DEVICE, /* A bare device */
> + MOUNTABLE_BTRFSVOL, /* A btrfs subvolume: device + volume */
> + MOUNTABLE_PATH /* An already mounted path: device = path */
> +} mountable_type_t;
> +
> +#endif /* GUESTFS_MOUNTABLE_H_ */
As discussed above, I don't believe this header is needed.
> diff --git a/tests/mountable/Makefile.am b/tests/mountable/Makefile.am
> new file mode 100644
> index 0000000..d50947f
> --- /dev/null
> +++ b/tests/mountable/Makefile.am
> @@ -0,0 +1,38 @@
> +# libguestfs
> +# Copyright (C) 2012 Red Hat Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program 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 General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +include $(top_srcdir)/subdir-rules.mk
> +
> +TESTS_ENVIRONMENT = $(top_builddir)/run --test
> +
> +TESTS=test-internal_parse_mountable
> +check_PROGRAMS = test-internal_parse_mountable
As discussed above, test-internal-parse-mountable would be consistent
with other code.
> +test_internal_parse_mountable_SOURCES = test-internal_parse_mountable.c
> +test_internal_parse_mountable_CFLAGS = \
> + -DGUESTFS_WARN_DEPRECATED=1 \
> + -DGUESTFS_PRIVATE=1 \
> + -I$(top_srcdir)/gnulib/lib -I$(top_builddir)/gnulib/lib \
> + -I$(top_srcdir)/src -I$(top_builddir)/src \
> + $(WARN_CFLAGS) $(WERROR_CFLAGS) \
> + $(GPROF_CFLAGS) $(GCOV_CFLAGS)
> +test_internal_parse_mountable_LDADD = \
> + $(top_builddir)/src/libguestfs.la \
> + $(top_builddir)/gnulib/lib/libgnu.la
> +
> +check-slow:
> + $(MAKE) TESTS="test-parallel" check
I think you forgot to remove the check-slow rule.
> diff --git a/tests/mountable/test-internal_parse_mountable.c b/tests/mountable/test-internal_parse_mountable.c
> new file mode 100644
> index 0000000..0e7593d
> --- /dev/null
> +++ b/tests/mountable/test-internal_parse_mountable.c
> @@ -0,0 +1,156 @@
> +/* libguestfs
> + * Copyright (C) 2012 Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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 General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <config.h>
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "guestfs.h"
> +
> +#define STREQ(a,b) (strcmp((a),(b)) == 0)
You shouldn't define this. Instead, include "guestfs-internal-frontend.h".
> +#define IMG "test.img"
> +
> +int
> +main (int argc, char *argv[])
> +{
> + int fd = open (IMG, O_WRONLY | O_CREAT | O_TRUNC, 0600);
> + if (fd == -1) {
> + perror ("open " IMG);
> + exit (EXIT_FAILURE);
> + }
> +
> + int r = posix_fallocate (fd, 0, 1024*1024*1024);
> + if (r != 0) {
> + fprintf (stderr, "posix_fallocate " IMG " 1G: %s\n", strerror (r));
> + unlink (IMG);
> + exit (EXIT_FAILURE);
> + }
> +
> + if (close (fd) == -1) {
> + perror ("close " IMG);
> + unlink (IMG);
> + exit (EXIT_FAILURE);
> + }
> +
> + guestfs_h *g = guestfs_create ();
> + if (g == NULL) {
> + fprintf (stderr, "guestfs_create: %s\n", guestfs_last_error (g));
> + exit (EXIT_FAILURE);
> + }
> +
> + if (guestfs_add_drive_opts (g, IMG,
> + GUESTFS_ADD_DRIVE_OPTS_FORMAT, "raw",
> + GUESTFS_ADD_DRIVE_OPTS_READONLY, 1,
> + -1) == -1) {
> + fprintf (stderr, "guestfs_add_drive_opts: %s\n", guestfs_last_error (g));
> +
> + error:
> + guestfs_close (g);
> + unlink (IMG);
> + exit (EXIT_FAILURE);
> + }
> +
> + if (guestfs_launch (g) == -1) {
> + fprintf (stderr, "guestfs_launch: %s\n", guestfs_last_error (g));
You shouldn't call fprintf here. libguestfs itself will print the
error on stderr, since you didn't override the default error handler
(and you shouldn't, in simple command line programs like this test).
> + goto error;
> + }
> +
> + if (guestfs_part_disk (g, "/dev/sda", "mbr") == -1) {
> + fprintf (stderr, "guestfs_part_disk: %s\n", guestfs_last_error (g));
> + goto error;
> + }
> +
> + if (guestfs_pvcreate (g, "/dev/sda1") == -1) {
> + fprintf (stderr, "guestfs_pvcreate: %s\n", guestfs_last_error (g));
> + goto error;
> + }
> +
> + const char *pvs[] = { "/dev/sda1", NULL };
> + if (guestfs_vgcreate (g, "VG", (char **) pvs) == -1) {
> + fprintf (stderr, "guestfs_vgcreate: %s\n", guestfs_last_error (g));
> + goto error;
> + }
> +
> + if (guestfs_lvcreate (g, "LV", "VG", 900) == -1) {
> + fprintf (stderr, "guestfs_lvcreate: %s\n", guestfs_last_error (g));
> + goto error;
> + }
> +
> + const char *devices[] = { "/dev/VG/LV", NULL };
> + if (guestfs_mkfs_btrfs (g, (char * const *)devices, -1) == -1) {
> + fprintf (stderr, "guestfs_mkfs_btrfs: %s\n", guestfs_last_error (g));
> + goto error;
> + }
> +
> + if (guestfs_mount (g, "/dev/VG/LV", "/") == -1) {
> + fprintf (stderr, "guestfs_mount: %s\n", guestfs_last_error (g));
> + goto error;
> + }
> +
> + if (guestfs_btrfs_subvolume_create (g, "/sv") == -1) {
> + fprintf (stderr, "guestfs_btrfs_subvolume_create: %s\n",
> + guestfs_last_error (g));
> + goto error;
> + }
> +
> + #if 0
> + struct guestfs_mountable_internal *mountable =
> + guestfs_internal_parse_mountable (g, "/dev/VG/LV");
> + if (mountable == NULL) {
> + fprintf (stderr, "guestfs_mountable_internal /dev/VG/LV: %s\n",
> + guestfs_last_error (g));
> + goto error;
> + }
> +
> + if (mountable->type != MOUNTABLE_DEVICE ||
> + !STREQ ("/dev/VG/LV", mountable->device))
> + {
> + fprintf (stderr, "incorrectly parsed /dev/VG/LV");
> + goto error;
> + }
> +
> + guestfs_free_mountable_internal (mountable);
> + #endif
> +
> + struct guestfs_mountable_internal *mountable =
> + guestfs_internal_parse_mountable (g, "btrfsvol:/dev/VG/LV/sv");
> + if (mountable == NULL) {
> + fprintf (stderr, "guestfs_mountable_internal /dev/VG/LV/sv: %s\n",
> + guestfs_last_error (g));
> + goto error;
> + }
> +
> + if (mountable->type != MOUNTABLE_BTRFSVOL ||
> + !STREQ ("/dev/VG/LV", mountable->device) ||
> + !STREQ ("sv", mountable->volume))
> + {
> + fprintf (stderr, "incorrectly parsed /dev/VG/LV/sv");
> + goto error;
> + }
> + guestfs_free_mountable_internal (mountable);
> +
> + guestfs_close (g);
> + unlink (IMG);
> +
> + exit (EXIT_SUCCESS);
> +}
> --
> 1.8.1.2
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
More information about the Libguestfs
mailing list