thr3ads.net - Libguestfs - Re: [Libguestfs] [PATCH 2/2] actions: refactor available & feature

If this information is useful, please help other people find it:
Share via:

Pino Toscano

2015-Nov-05 15:56 UTC

[Libguestfs] [PATCH 1/2] actions: turn available & feature_available as non-daemon

Rename the current available and feature_available into internal daemon
functions, and provide non-daemon functions wrapping them at library
side.  This will make it possible to e.g. add caching for them.

Should be only refactoring, no actual behaviour change.
---
 daemon/available.c   |   4 +-
 generator/actions.ml | 192 ++++++++++++++++++++++++++++-----------------------
 po/POTFILES          |   1 +
 src/MAX_PROC_NR      |   2 +-
 src/Makefile.am      |   1 +
 src/available.c      |  34 +++++++++
 6 files changed, 143 insertions(+), 91 deletions(-)
 create mode 100644 src/available.c

diff --git a/daemon/available.c b/daemon/available.c
index 54c6b9b..6409b90 100644
--- a/daemon/available.c
+++ b/daemon/available.c
@@ -67,13 +67,13 @@ available (char *const *groups, int error_on_unavailable)
 }
 
 int
-do_feature_available (char *const *groups)
+do_internal_feature_available (char *const *groups)
 {
   return available (groups, 0);
 }
 
 int
-do_available (char *const *groups)
+do_internal_available (char *const *groups)
 {
   if (available (groups, 1) == -1)
     return -1;
diff --git a/generator/actions.ml b/generator/actions.ml
index 7ecd384..6348c88 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -3412,6 +3412,92 @@ C<guestfs_get_identifier>." };
     longdesc = "\
 Get the handle identifier.  See C<guestfs_set_identifier>." };
 
+  { defaults with
+    name = "available"; added = (1, 0, 80);
+    style = RErr, [StringList "groups"], [];
+    tests = [
+      InitNone, Always, TestRun [["available"; ""]], []
+    ];
+    shortdesc = "test availability of some parts of the API";
+    longdesc = "\
+This command is used to check the availability of some
+groups of functionality in the appliance, which not all builds of
+the libguestfs appliance will be able to provide.
+
+The libguestfs groups, and the functions that those
+groups correspond to, are listed in L<guestfs(3)/AVAILABILITY>.
+You can also fetch this list at runtime by calling
+C<guestfs_available_all_groups>.
+
+The argument C<groups> is a list of group names, eg:
+C<[\"inotify\", \"augeas\"]> would check for the
availability of
+the Linux inotify functions and Augeas (configuration file
+editing) functions.
+
+The command returns no error if I<all> requested groups are available.
+
+It fails with an error if one or more of the requested
+groups is unavailable in the appliance.
+
+If an unknown group name is included in the
+list of groups then an error is always returned.
+
+I<Notes:>
+
+=over 4
+
+=item *
+
+C<guestfs_feature_available> is the same as this call, but
+with a slightly simpler to use API: that call returns a boolean
+true/false instead of throwing an error.
+
+=item *
+
+You must call C<guestfs_launch> before calling this function.
+
+The reason is because we don't know what groups are
+supported by the appliance/daemon until it is running and can
+be queried.
+
+=item *
+
+If a group of functions is available, this does not necessarily
+mean that they will work.  You still have to check for errors
+when calling individual API functions even if they are
+available.
+
+=item *
+
+It is usually the job of distro packagers to build
+complete functionality into the libguestfs appliance.
+Upstream libguestfs, if built from source with all
+requirements satisfied, will support everything.
+
+=item *
+
+This call was added in version C<1.0.80>.  In previous
+versions of libguestfs all you could do would be to speculatively
+execute a command to find out if the daemon implemented it.
+See also C<guestfs_version>.
+
+=back
+
+See also C<guestfs_filesystem_available>." };
+
+  { defaults with
+    name = "feature_available"; added = (1, 21, 26);
+    style = RBool "isavailable", [StringList "groups"], [];
+    tests = [
+      InitNone, Always, TestResultTrue [["feature_available";
""]], []
+    ];
+    shortdesc = "test availability of some parts of the API";
+    longdesc = "\
+This is the same as C<guestfs_available>, but unlike that
+call it returns a simple true/false boolean result, instead
+of throwing an exception if a feature is not found.  For
+other documentation see C<guestfs_available>." };
+
 ]
 
 (* daemon_functions are any functions which cause some action
@@ -8031,80 +8117,6 @@ To create a file with a pattern of repeating bytes
 use C<guestfs_fill_pattern>." };
 
   { defaults with
-    name = "available"; added = (1, 0, 80);
-    style = RErr, [StringList "groups"], [];
-    proc_nr = Some 216;
-    tests = [
-      InitNone, Always, TestRun [["available"; ""]], []
-    ];
-    shortdesc = "test availability of some parts of the API";
-    longdesc = "\
-This command is used to check the availability of some
-groups of functionality in the appliance, which not all builds of
-the libguestfs appliance will be able to provide.
-
-The libguestfs groups, and the functions that those
-groups correspond to, are listed in L<guestfs(3)/AVAILABILITY>.
-You can also fetch this list at runtime by calling
-C<guestfs_available_all_groups>.
-
-The argument C<groups> is a list of group names, eg:
-C<[\"inotify\", \"augeas\"]> would check for the
availability of
-the Linux inotify functions and Augeas (configuration file
-editing) functions.
-
-The command returns no error if I<all> requested groups are available.
-
-It fails with an error if one or more of the requested
-groups is unavailable in the appliance.
-
-If an unknown group name is included in the
-list of groups then an error is always returned.
-
-I<Notes:>
-
-=over 4
-
-=item *
-
-C<guestfs_feature_available> is the same as this call, but
-with a slightly simpler to use API: that call returns a boolean
-true/false instead of throwing an error.
-
-=item *
-
-You must call C<guestfs_launch> before calling this function.
-
-The reason is because we don't know what groups are
-supported by the appliance/daemon until it is running and can
-be queried.
-
-=item *
-
-If a group of functions is available, this does not necessarily
-mean that they will work.  You still have to check for errors
-when calling individual API functions even if they are
-available.
-
-=item *
-
-It is usually the job of distro packagers to build
-complete functionality into the libguestfs appliance.
-Upstream libguestfs, if built from source with all
-requirements satisfied, will support everything.
-
-=item *
-
-This call was added in version C<1.0.80>.  In previous
-versions of libguestfs all you could do would be to speculatively
-execute a command to find out if the daemon implemented it.
-See also C<guestfs_version>.
-
-=back
-
-See also C<guestfs_filesystem_available>." };
-
-  { defaults with
     name = "dd"; added = (1, 0, 80);
     style = RErr, [Dev_or_Path "src"; Dev_or_Path "dest"],
[];
     proc_nr = Some 217;
@@ -11763,20 +11775,6 @@ This is only used to debug RHBZ#914931.  Note that this
 deliberately crashes guestfsd." };
 
   { defaults with
-    name = "feature_available"; added = (1, 21, 26);
-    style = RBool "isavailable", [StringList "groups"], [];
-    proc_nr = Some 398;
-    tests = [
-      InitNone, Always, TestResultTrue [["feature_available";
""]], []
-    ];
-    shortdesc = "test availability of some parts of the API";
-    longdesc = "\
-This is the same as C<guestfs_available>, but unlike that
-call it returns a simple true/false boolean result, instead
-of throwing an exception if a feature is not found.  For
-other documentation see C<guestfs_available>." };
-
-  { defaults with
     name = "syslinux"; added = (1, 21, 27);
     style = RErr, [Device "device"], [OString "directory"];
     proc_nr = Some 399;
@@ -12780,6 +12778,24 @@ this will fail and set errno as ENOTSUP.
 
 See also L<ntfsresize(8)>, L<resize2fs(8)>, L<btrfs(8)>,
L<xfs_info(8)>." };
 
+  { defaults with
+    name = "internal_available"; added = (1, 31, 25);
+    style = RErr, [StringList "groups"], [];
+    proc_nr = Some 458;
+    visibility = VInternal;
+    shortdesc = "test availability of some parts of the API";
+    longdesc = "\
+This is the internal call which implements C<guestfs_available>." };
+
+  { defaults with
+    name = "internal_feature_available"; added = (1, 31, 25);
+    style = RBool "isavailable", [StringList "groups"], [];
+    proc_nr = Some 459;
+    visibility = VInternal;
+    shortdesc = "test availability of some parts of the API";
+    longdesc = "\
+This is the internal call which implements
C<guestfs_feature_available>." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
diff --git a/po/POTFILES b/po/POTFILES
index 32f88a1..d87d668 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -300,6 +300,7 @@ src/actions-support.c
 src/actions-variants.c
 src/alloc.c
 src/appliance.c
+src/available.c
 src/bindtests.c
 src/canonical-name.c
 src/cleanup.c
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index de2a00c..658bd78 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-457
+459
diff --git a/src/Makefile.am b/src/Makefile.am
index 4515c4d..25c6fa3 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -83,6 +83,7 @@ libguestfs_la_SOURCES = \
 	actions-variants.c \
 	alloc.c \
 	appliance.c \
+	available.c \
 	bindtests.c \
 	canonical-name.c \
 	command.c \
diff --git a/src/available.c b/src/available.c
new file mode 100644
index 0000000..6738449
--- /dev/null
+++ b/src/available.c
@@ -0,0 +1,34 @@
+/* libguestfs
+ * Copyright (C) 2015 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
+ */
+
+#include <config.h>
+
+#include "guestfs.h"
+#include "guestfs-internal-actions.h"
+
+int
+guestfs_impl_available (guestfs_h *g, char *const *groups)
+{
+  return guestfs_internal_available (g, groups);
+}
+
+int
+guestfs_impl_feature_available (guestfs_h *g, char *const *groups)
+{
+  return guestfs_internal_feature_available (g, groups);
+}
-- 
2.1.0

Pino Toscano

2015-Nov-05 15:56 UTC

head link

[Libguestfs] [PATCH 2/2] actions: refactor available & feature_available

Refactor the internal_feature_available to return the result for just
one group, so it is easier to know on the library side what was the
actual error, and which group refers to; drop internal_available, as no
more needed after this.

On the library side, implement in available and feature_available the
real logic to iterate through the requested group, and error out or
return whether the groups are available. This also introduces caching
for the features, so each needs to be queried just once in each
appliance run.

The result of this is that there should be much less communication with
the daemon to know about available features; the downside is that
queries for more than one group at once, not already cached, will be
cause more queries to the daemon.
---
 daemon/available.c     | 51 ++++++----------------------------
 generator/actions.ml   | 13 ++-------
 src/MAX_PROC_NR        |  2 +-
 src/available.c        | 74 ++++++++++++++++++++++++++++++++++++++++++++++++--
 src/guestfs-internal.h | 10 +++++++
 src/handle.c           |  7 +++++
 6 files changed, 101 insertions(+), 56 deletions(-)

diff --git a/daemon/available.c b/daemon/available.c
index 6409b90..d50c737 100644
--- a/daemon/available.c
+++ b/daemon/available.c
@@ -33,53 +33,20 @@
 GUESTFSD_EXT_CMD(str_grep, grep);
 GUESTFSD_EXT_CMD(str_modprobe, modprobe);
 
-static int
-available (char *const *groups, int error_on_unavailable)
+int
+do_internal_feature_available (const char *group)
 {
-  int av;
-  size_t i, j;
-
-  for (i = 0; groups[i] != NULL; ++i) {
-    for (j = 0; optgroups[j].group != NULL; ++j) {
-      if (STREQ (groups[i], optgroups[j].group)) {
-        av = optgroups[j].available ();
-        if (!av) {
-          if (error_on_unavailable) {
-            reply_with_error ("%s: group not available",
optgroups[j].group);
-            return -1;
-          }
-          else
-            return 0;
-        }
-        break; /* out of for (j) loop */
-      }
-    }
+  size_t i;
 
-    /* Unknown group? */
-    if (optgroups[j].group == NULL) {
-      reply_with_error ("%s: unknown group", groups[i]);
-      return -1;
+  for (i = 0; optgroups[i].group != NULL; ++i) {
+    if (STREQ (group, optgroups[i].group)) {
+      int av = optgroups[i].available ();
+      return av ? 0 : 1;
     }
   }
 
-  /* All specified groups available. */
-  return 1;
-}
-
-int
-do_internal_feature_available (char *const *groups)
-{
-  return available (groups, 0);
-}
-
-int
-do_internal_available (char *const *groups)
-{
-  if (available (groups, 1) == -1)
-    return -1;
-
-  /* No error, so all groups available, just returns no error. */
-  return 0;
+  /* Unknown group */
+  return 2;
 }
 
 char **
diff --git a/generator/actions.ml b/generator/actions.ml
index 6348c88..7ab8ee4 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -12779,18 +12779,9 @@ this will fail and set errno as ENOTSUP.
 See also L<ntfsresize(8)>, L<resize2fs(8)>, L<btrfs(8)>,
L<xfs_info(8)>." };
 
   { defaults with
-    name = "internal_available"; added = (1, 31, 25);
-    style = RErr, [StringList "groups"], [];
-    proc_nr = Some 458;
-    visibility = VInternal;
-    shortdesc = "test availability of some parts of the API";
-    longdesc = "\
-This is the internal call which implements C<guestfs_available>." };
-
-  { defaults with
     name = "internal_feature_available"; added = (1, 31, 25);
-    style = RBool "isavailable", [StringList "groups"], [];
-    proc_nr = Some 459;
+    style = RInt "result", [String "group"], [];
+    proc_nr = Some 458;
     visibility = VInternal;
     shortdesc = "test availability of some parts of the API";
     longdesc = "\
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 658bd78..c92ddb6 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-459
+458
diff --git a/src/available.c b/src/available.c
index 6738449..4bdd173 100644
--- a/src/available.c
+++ b/src/available.c
@@ -18,17 +18,87 @@
 
 #include <config.h>
 
+#include <string.h>
+#include <libintl.h>
+
 #include "guestfs.h"
+#include "guestfs-internal.h"
 #include "guestfs-internal-actions.h"
 
+static struct cached_feature *
+find_or_cache_feature (guestfs_h *g, const char *group)
+{
+  struct cached_feature *f;
+  size_t i;
+  int res;
+
+  for (i = 0; i < g->nr_features; ++i) {
+    f = &g->features[i];
+
+    if (STRNEQ (f->group, group))
+      continue;
+
+    return f;
+  }
+
+  res = guestfs_internal_feature_available (g, group);
+  if (res < 0)
+    return 0;  /* internal_feature_available sent an error. */
+
+  g->features = safe_realloc (g, g->features,
+                              (g->nr_features+1) * sizeof (struct
cached_feature));
+  f = &g->features[g->nr_features];
+  ++g->nr_features;
+  f->group = safe_strdup (g, group);
+  f->result = res;
+
+  return f;
+}
+
 int
 guestfs_impl_available (guestfs_h *g, char *const *groups)
 {
-  return guestfs_internal_available (g, groups);
+  char *const *ptr;
+
+  for (ptr = groups; *ptr != NULL; ++ptr) {
+    const char *group = *ptr;
+    struct cached_feature *f = find_or_cache_feature (g, group);
+
+    if (f == NULL)
+      return -1;
+
+    if (f->result == 2) {
+      error (g, _("%s: unknown group"), group);
+      return -1;
+    } else if (f->result == 1) {
+      error (g, _("%s: group not available"), group);
+      return -1;
+    }
+  }
+
+  return 0;
 }
 
 int
 guestfs_impl_feature_available (guestfs_h *g, char *const *groups)
 {
-  return guestfs_internal_feature_available (g, groups);
+  char *const *ptr;
+
+  for (ptr = groups; *ptr != NULL; ++ptr) {
+    const char *group = *ptr;
+    struct cached_feature *f = find_or_cache_feature (g, group);
+
+    if (f == NULL)
+      return -1;
+
+    if (f->result == 2) {
+      error (g, _("%s: unknown group"), group);
+      return -1;
+    } else if (f->result == 1) {
+      return 0;
+    }
+  }
+
+  /* All specified groups available. */
+  return 1;
 }
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 49da6fe..bc03ccc 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -360,6 +360,12 @@ struct error_cb_stack {
   void *                   error_cb_data;
 };
 
+/* Cached queried features. */
+struct cached_feature {
+  char *group;
+  int result;
+};
+
 /* The libguestfs handle. */
 struct guestfs_h
 {
@@ -502,6 +508,10 @@ struct guestfs_h
   unsigned int nr_requested_credentials;
   virConnectCredentialPtr requested_credentials;
 #endif
+
+  /* Cached features. */
+  struct cached_feature *features;
+  size_t nr_features;
 };
 
 /* Per-filesystem data stored for inspect_os. */
diff --git a/src/handle.c b/src/handle.c
index bd66717..f7b0909 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -422,6 +422,7 @@ static int
 shutdown_backend (guestfs_h *g, int check_for_errors)
 {
   int ret = 0;
+  size_t i;
 
   if (g->state == CONFIG)
     return 0;
@@ -444,6 +445,12 @@ shutdown_backend (guestfs_h *g, int check_for_errors)
 
   guestfs_int_free_drives (g);
 
+  for (i = 0; i < g->nr_features; ++i)
+    free (g->features[i].group);
+  free (g->features);
+  g->features = NULL;
+  g->nr_features = 0;
+
   g->state = CONFIG;
 
   return ret;
-- 
2.1.0

Richard W.M. Jones

2015-Nov-05 16:01 UTC

head link

Re: [Libguestfs] [PATCH 1/2] actions: turn available & feature_available as non-daemon

On Thu, Nov 05, 2015 at 04:56:42PM +0100, Pino Toscano
wrote:> Rename the current available and feature_available into internal daemon
> functions, and provide non-daemon functions wrapping them at library
> side.  This will make it possible to e.g. add caching for them.
> 
> Should be only refactoring, no actual behaviour change.
Looks good - ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Richard W.M. Jones

2015-Nov-05 16:04 UTC

head link

Re: [Libguestfs] [PATCH 2/2] actions: refactor available & feature_available

On Thu, Nov 05, 2015 at 04:56:43PM +0100, Pino Toscano
wrote:> Refactor the internal_feature_available to return the result for just
> one group, so it is easier to know on the library side what was the
> actual error, and which group refers to; drop internal_available, as no
> more needed after this.
> 
> On the library side, implement in available and feature_available the
> real logic to iterate through the requested group, and error out or
> return whether the groups are available. This also introduces caching
> for the features, so each needs to be queried just once in each
> appliance run.
> 
> The result of this is that there should be much less communication with
> the daemon to know about available features; the downside is that
> queries for more than one group at once, not already cached, will be
> cause more queries to the daemon.
Looks OK, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Richard W.M. Jones

2015-Nov-06 21:40 UTC

head link

Re: [Libguestfs] [PATCH 2/2] actions: refactor available & feature_available

On Thu, Nov 05, 2015 at 04:56:43PM +0100, Pino Toscano
wrote:> Refactor the internal_feature_available to return the result for just
> one group, so it is easier to know on the library side what was the
> actual error, and which group refers to; drop internal_available, as no
> more needed after this.
> 
> On the library side, implement in available and feature_available the
> real logic to iterate through the requested group, and error out or
> return whether the groups are available. This also introduces caching
> for the features, so each needs to be queried just once in each
> appliance run.
> 
> The result of this is that there should be much less communication with
> the daemon to know about available features; the downside is that
> queries for more than one group at once, not already cached, will be
> cause more queries to the daemon.
This commit causes the following valgrind failures.  Curiously they
only occur in virt-v2v:

==25905== HEAP SUMMARY:
==25905==     in use at exit: 3,604,364 bytes in 1,663 blocks
==25905==   total heap usage: 83,930 allocs, 82,267 frees, 4,200,256,957 bytes
allocated
==25905== 
==25905== 41 (32 direct, 9 indirect) bytes in 1 blocks are definitely lost in
loss record 696 of 775
==25905==    at 0x4C2AB9D: realloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==25905==    by 0x4ECF846: guestfs_int_safe_realloc (alloc.c:80)
==25905==    by 0x4ED05FF: find_or_cache_feature (available.c:48)
==25905==    by 0x4ED073C: guestfs_impl_feature_available (available.c:89)
==25905==    by 0x4EA4B21: guestfs_feature_available (actions-5.c:1131)
==25905==    by 0x4EF9FDF: guestfs_impl_list_filesystems (listfs.c:49)
==25905==    by 0x4E7EDF2: guestfs_list_filesystems (actions-3.c:419)
==25905==    by 0x4EE0FD2: guestfs_impl_inspect_os (inspect.c:62)
==25905==    by 0x4E50E12: guestfs_inspect_os (actions-0.c:500)
==25905==    by 0x4E3292: ocaml_guestfs_inspect_os (guestfs-c-actions.c:9385)
==25905==    by 0x4696C8: camlGuestfs__fun_16039 (guestfs.ml:1242)
==25905==    by 0x423224: camlV2v__inspect_source_1085 (v2v.ml:306)

I've been staring at the code for a long while, and I cannot see what
the problem is ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Maybe Matching Threads

Search for more maybe matching threads

Libguestfs - Nov 2015 - Re: [PATCH 2/2] actions: refactor available & feature_available

[Libguestfs] [PATCH 1/2] actions: turn available & feature_available as non-daemon

[Libguestfs] [PATCH 2/2] actions: refactor available & feature_available

Re: [Libguestfs] [PATCH 1/2] actions: turn available & feature_available as non-daemon

Re: [Libguestfs] [PATCH 2/2] actions: refactor available & feature_available

Re: [Libguestfs] [PATCH 2/2] actions: refactor available & feature_available

Maybe Matching Threads