[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
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
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
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
On Fri, Nov 06, 2015 at 09:40:53PM +0000, Richard W.M. Jones wrote:
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 ...
It's officially driving me nuts. I'm beginning to wonder if it's memory corruption, but perhaps it's just because it's late at night. The reproducer is simple enough: $ make -C test-data check $ ./run valgrind --log-file=/tmp/log \ --suppressions=valgrind-suppressions \ --leak-check=full \ virt-v2v -ic test:///$(pwd)/test-data/phony-guests/guests.xml windows \ -o null $ less /tmp/log (/tmp/log is attached) 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
On Friday 06 November 2015 21:40:53 Richard W.M. Jones wrote:
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:
I can reproduce it also on other tools, and I found the issue of that. Patch(es) will come in a minute. Thanks for pointing it out, -- Pino Toscano
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
participants (2)
-
Pino Toscano -
Richard W.M. Jones