[PATCH] appliance: reorder the steps to search for appliance
The patch changes the order of the steps to search for fixed/supermin appliance in accordance with documentation: "If the fixed appliance is found, libguestfs skips supermin entirely and just runs qemu with the kernel, initrd and root disk from the fixed appliance." Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- lib/appliance.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/appliance.c b/lib/appliance.c index f12918573..06ee06f71 100644 --- a/lib/appliance.c +++ b/lib/appliance.c @@ -137,16 +137,6 @@ build_appliance (guestfs_h *g, CLEANUP_FREE char *path = NULL; /* Step (1). */ - r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); - if (r == -1) - return -1; - - if (r == 1) - /* Step (2): build supermin appliance. */ - return build_supermin_appliance (g, supermin_path, - kernel, initrd, appliance); - - /* Step (3). */ r = find_path (g, contains_fixed_appliance, NULL, &path); if (r == -1) return -1; @@ -162,7 +152,7 @@ build_appliance (guestfs_h *g, return 0; } - /* Step (4). */ + /* Step (2). */ r = find_path (g, contains_old_style_appliance, NULL, &path); if (r == -1) return -1; @@ -177,6 +167,16 @@ build_appliance (guestfs_h *g, return 0; } + /* Step (3). */ + r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); + if (r == -1) + return -1; + + if (r == 1) + /* Step (4): build supermin appliance. */ + return build_supermin_appliance (g, supermin_path, + kernel, initrd, appliance); + error (g, _("cannot find any suitable libguestfs supermin, fixed or old-style appliance on LIBGUESTFS_PATH (search path: %s)"), g->path); return -1; -- 2.11.0
On Tue, Apr 25, 2017 at 02:35:26PM +0300, Pavel Butsykin wrote:
The patch changes the order of the steps to search for fixed/supermin appliance in accordance with documentation:
"If the fixed appliance is found, libguestfs skips supermin entirely and just runs qemu with the kernel, initrd and root disk from the fixed appliance."
Does anyone rely on the path-like behaviour of LIBGUESTFS_PATH? It was a mistake to allow that originally, but we're stuck with it now unfortunately.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- lib/appliance.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/lib/appliance.c b/lib/appliance.c index f12918573..06ee06f71 100644 --- a/lib/appliance.c +++ b/lib/appliance.c @@ -137,16 +137,6 @@ build_appliance (guestfs_h *g, CLEANUP_FREE char *path = NULL;
/* Step (1). */ - r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); - if (r == -1) - return -1; - - if (r == 1) - /* Step (2): build supermin appliance. */ - return build_supermin_appliance (g, supermin_path, - kernel, initrd, appliance); - - /* Step (3). */ r = find_path (g, contains_fixed_appliance, NULL, &path); if (r == -1) return -1; @@ -162,7 +152,7 @@ build_appliance (guestfs_h *g, return 0; }
- /* Step (4). */ + /* Step (2). */
The steps actually refer to the documentation. See in the same file, starting from " * Locate or build the appliance." Rich.
r = find_path (g, contains_old_style_appliance, NULL, &path); if (r == -1) return -1; @@ -177,6 +167,16 @@ build_appliance (guestfs_h *g, return 0; }
+ /* Step (3). */ + r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); + if (r == -1) + return -1; + + if (r == 1) + /* Step (4): build supermin appliance. */ + return build_supermin_appliance (g, supermin_path, + kernel, initrd, appliance); + error (g, _("cannot find any suitable libguestfs supermin, fixed or old-style appliance on LIBGUESTFS_PATH (search path: %s)"), g->path); return -1; -- 2.11.0
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 25.04.2017 15:11, Richard W.M. Jones wrote:
On Tue, Apr 25, 2017 at 02:35:26PM +0300, Pavel Butsykin wrote:
The patch changes the order of the steps to search for fixed/supermin appliance in accordance with documentation:
"If the fixed appliance is found, libguestfs skips supermin entirely and just runs qemu with the kernel, initrd and root disk from the fixed appliance."
Does anyone rely on the path-like behaviour of LIBGUESTFS_PATH? It was a mistake to allow that originally, but we're stuck with it now unfortunately.
Sorry, but I don't understand what you mean. What is "path-like behaviour"? What is the mistake ? The mistake is to use the location of supermin.d directory for the decision on build appliance.? I was faced with the problem that if in appliance directory there is supermin.d then libguestfs is trying to build appliance. Even if supermin.d is empty, even if libguestfs was built with options --disable-appliance --disable-daemon. But as it turned out, the behaviour build_appliance() is contrary to the documentation.
Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- lib/appliance.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/lib/appliance.c b/lib/appliance.c index f12918573..06ee06f71 100644 --- a/lib/appliance.c +++ b/lib/appliance.c @@ -137,16 +137,6 @@ build_appliance (guestfs_h *g, CLEANUP_FREE char *path = NULL;
/* Step (1). */ - r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); - if (r == -1) - return -1; - - if (r == 1) - /* Step (2): build supermin appliance. */ - return build_supermin_appliance (g, supermin_path, - kernel, initrd, appliance); - - /* Step (3). */ r = find_path (g, contains_fixed_appliance, NULL, &path); if (r == -1) return -1; @@ -162,7 +152,7 @@ build_appliance (guestfs_h *g, return 0; }
- /* Step (4). */ + /* Step (2). */
The steps actually refer to the documentation. See in the same file, starting from " * Locate or build the appliance."
It seems that I missed it, thanks.
Rich.
r = find_path (g, contains_old_style_appliance, NULL, &path); if (r == -1) return -1; @@ -177,6 +167,16 @@ build_appliance (guestfs_h *g, return 0; }
+ /* Step (3). */ + r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); + if (r == -1) + return -1; + + if (r == 1) + /* Step (4): build supermin appliance. */ + return build_supermin_appliance (g, supermin_path, + kernel, initrd, appliance); + error (g, _("cannot find any suitable libguestfs supermin, fixed or old-style appliance on LIBGUESTFS_PATH (search path: %s)"), g->path); return -1; -- 2.11.0
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Tue, Apr 25, 2017 at 03:58:45PM +0300, Pavel Butsykin wrote:
On 25.04.2017 15:11, Richard W.M. Jones wrote:
On Tue, Apr 25, 2017 at 02:35:26PM +0300, Pavel Butsykin wrote:
The patch changes the order of the steps to search for fixed/supermin appliance in accordance with documentation:
"If the fixed appliance is found, libguestfs skips supermin entirely and just runs qemu with the kernel, initrd and root disk from the fixed appliance."
Does anyone rely on the path-like behaviour of LIBGUESTFS_PATH? It was a mistake to allow that originally, but we're stuck with it now unfortunately.
Sorry, but I don't understand what you mean. What is "path-like behaviour"? What is the mistake ? The mistake is to use the location of supermin.d directory for the decision on build appliance.?
I mean, the mistake was that I allowed search paths in LIBGUESTFS_PATH, ie. LIBGUESTFS_PATH=/a:/b (rather than just forcing it to be a single path).
I was faced with the problem that if in appliance directory there is supermin.d then libguestfs is trying to build appliance. Even if supermin.d is empty, even if libguestfs was built with options --disable-appliance --disable-daemon. But as it turned out, the behaviour build_appliance() is contrary to the documentation.
Can you see what: guestfish get-path prints? Are you setting LIBGUESTFS_PATH at all? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 25.04.2017 16:04, Richard W.M. Jones wrote:
On Tue, Apr 25, 2017 at 03:58:45PM +0300, Pavel Butsykin wrote:
On 25.04.2017 15:11, Richard W.M. Jones wrote:
On Tue, Apr 25, 2017 at 02:35:26PM +0300, Pavel Butsykin wrote:
The patch changes the order of the steps to search for fixed/supermin appliance in accordance with documentation:
"If the fixed appliance is found, libguestfs skips supermin entirely and just runs qemu with the kernel, initrd and root disk from the fixed appliance."
Does anyone rely on the path-like behaviour of LIBGUESTFS_PATH? It was a mistake to allow that originally, but we're stuck with it now unfortunately.
Sorry, but I don't understand what you mean. What is "path-like behaviour"? What is the mistake ? The mistake is to use the location of supermin.d directory for the decision on build appliance.?
I mean, the mistake was that I allowed search paths in LIBGUESTFS_PATH, ie.
LIBGUESTFS_PATH=/a:/b
(rather than just forcing it to be a single path).
I was faced with the problem that if in appliance directory there is supermin.d then libguestfs is trying to build appliance. Even if supermin.d is empty, even if libguestfs was built with options --disable-appliance --disable-daemon. But as it turned out, the behaviour build_appliance() is contrary to the documentation.
Can you see what:
guestfish get-path
/usr/lib64/guestfs
prints? Are you setting LIBGUESTFS_PATH at all?
No. # ls /usr/lib64/guestfs initrd kernel README.fixed root supermin.d libguestfs by default uses a single path to search fixed appliance and supermin.d. It seemed to me that the presence of options "--disable appliance --disable-daemon" must exclude the use of build_supermin_appliance. But libguestfs in any case trying to find supermin.d. Maybe it's not quite right to make such decisions based on the contents of the directory. Also, I still don't understand what the priority of the search appliances was supposed to actually use. Because in the documentation the first step is to search for fixed appliance, but is actually supermin.d(for building appliance).
Rich.
On Tue, Apr 25, 2017 at 06:13:37PM +0300, Pavel Butsykin wrote:
On 25.04.2017 16:04, Richard W.M. Jones wrote:
Can you see what:
guestfish get-path
/usr/lib64/guestfs
prints? Are you setting LIBGUESTFS_PATH at all?
No.
# ls /usr/lib64/guestfs initrd kernel README.fixed root supermin.d
libguestfs by default uses a single path to search fixed appliance and supermin.d. It seemed to me that the presence of options "--disable appliance --disable-daemon" must exclude the use of build_supermin_appliance. But libguestfs in any case trying to find supermin.d.
OK, I see - what's actually happening is that you've got a path which is both a fixed appliance and a supermin appliance (although maybe it only appears to be a supermin appliance -- libguestfs just looks for the "supermin.d" entry and decides it's a supermin appliance).
Maybe it's not quite right to make such decisions based on the contents of the directory.
Yes that's right, but see below.
Also, I still don't understand what the priority of the search appliances was supposed to actually use. Because in the documentation the first step is to search for fixed appliance, but is actually supermin.d(for building appliance).
There's not a priority in this situation. It wasn't intended that two different appliances would be on the exact same path. Some possible solutions: (1) Make the check in lib/appliance.c: contains_supermin_appliance more robust. It could perhaps be changed so that it checks that at least "base.tar.gz" and "packages" are found within the supermin.d directory. Those are the minimum two files that must be in a supermin appliance (see supermin(8)). (2) If the fixed appliance was located somewhere else, you could do: LIBGUESTFS_PATH=/path/to/somewhere/else:/usr/lib64/guestfs export LIBGUESTFS_PATH and then it would look for the fixed appliance in /path/to/somewhere/else and use it. If the fixed appliance exists there, then it would never check /usr/lib64/guestfs. (3) You could also compile a different path into libguestfs. Unfortunately there's no way to specify it at configure time, but see GUESTFS_DEFAULT_PATH in the sources. (4) Delete /usr/lib64/guestfs/supermin.d Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On 25.04.2017 18:33, Richard W.M. Jones wrote:
On Tue, Apr 25, 2017 at 06:13:37PM +0300, Pavel Butsykin wrote:
On 25.04.2017 16:04, Richard W.M. Jones wrote:
Can you see what:
guestfish get-path
/usr/lib64/guestfs
prints? Are you setting LIBGUESTFS_PATH at all?
No.
# ls /usr/lib64/guestfs initrd kernel README.fixed root supermin.d
libguestfs by default uses a single path to search fixed appliance and supermin.d. It seemed to me that the presence of options "--disable appliance --disable-daemon" must exclude the use of build_supermin_appliance. But libguestfs in any case trying to find supermin.d.
OK, I see - what's actually happening is that you've got a path which is both a fixed appliance and a supermin appliance (although maybe it only appears to be a supermin appliance -- libguestfs just looks for the "supermin.d" entry and decides it's a supermin appliance).
Maybe it's not quite right to make such decisions based on the contents of the directory.
Yes that's right, but see below.
Moreover, we have a situation with undefined behavior. if we only use the fixed appliance, then we can't rely on the contents of directories, because there are many ways to stumble on supermin.d
Also, I still don't understand what the priority of the search appliances was supposed to actually use. Because in the documentation the first step is to search for fixed appliance, but is actually supermin.d(for building appliance).
There's not a priority in this situation. It wasn't intended that two different appliances would be on the exact same path.
As I see using the same path for different appliances is not a problem. Because build_appliance() first looks for the supermin.d in the g->path, and then the fixed appliance in the same g->path. Therefore, setting the two paths into LIBGUESTFS_PATH=/lib64/guestfs/appliance2: /lib64/guestfs doesn't help.
Some possible solutions:
(1) Make the check in lib/appliance.c: contains_supermin_appliance more robust.
It could perhaps be changed so that it checks that at least "base.tar.gz" and "packages" are found within the supermin.d directory. Those are the minimum two files that must be in a supermin appliance (see supermin(8)).
Empty supermin.d is not a problem in this situation, supermin.d with content may belong to a different libguestfs version or come from arbitrary utilities/packages.
(2) If the fixed appliance was located somewhere else, you could do:
LIBGUESTFS_PATH=/path/to/somewhere/else:/usr/lib64/guestfs export LIBGUESTFS_PATH
and then it would look for the fixed appliance in /path/to/somewhere/else and use it. If the fixed appliance exists there, then it would never check /usr/lib64/guestfs.
It's a bit wrong, the first step is search supermin.d in all specified directories: /* Step (1). */ r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); if (r == -1) return -1; if (r == 1) /* Step (2): build supermin appliance. */ return build_supermin_appliance (g, supermin_path, kernel, initrd, appliance); ... So, supermin.d will be found in /usr/lib64/guestfs and the search for the fixed applaince will not even. Here find_path() searches for all paths in g->path.
(3) You could also compile a different path into libguestfs.
Unfortunately there's no way to specify it at configure time, but see GUESTFS_DEFAULT_PATH in the sources.
(4) Delete /usr/lib64/guestfs/supermin.d
It can even be dangerous, because supermin.d may belong to any packages or utilities. I propose to consider a patch that adds some certainty of behaviour in the work with appliances, and solves the above notation issue. What do you think? (see path in the next message)
Rich.
On Thu, Apr 27, 2017 at 03:54:44PM +0300, Pavel Butsykin wrote: [...]
(2) If the fixed appliance was located somewhere else, you could do:
LIBGUESTFS_PATH=/path/to/somewhere/else:/usr/lib64/guestfs export LIBGUESTFS_PATH
and then it would look for the fixed appliance in /path/to/somewhere/else and use it. If the fixed appliance exists there, then it would never check /usr/lib64/guestfs.
It's a bit wrong, the first step is search supermin.d in all specified directories: /* Step (1). */ r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); if (r == -1) return -1;
if (r == 1) /* Step (2): build supermin appliance. */ return build_supermin_appliance (g, supermin_path, kernel, initrd, appliance);
OK, I see, that is indeed a bug. Can you suggest a patch which fixes this so that directories are searched correctly? However I still think that putting all of the appliances into a single directory and expecting libguestfs to know which one you meant it to choose first is the wrong idea. 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 27.04.2017 17:43, Richard W.M. Jones wrote:
On Thu, Apr 27, 2017 at 03:54:44PM +0300, Pavel Butsykin wrote: [...]
(2) If the fixed appliance was located somewhere else, you could do:
LIBGUESTFS_PATH=/path/to/somewhere/else:/usr/lib64/guestfs export LIBGUESTFS_PATH
and then it would look for the fixed appliance in /path/to/somewhere/else and use it. If the fixed appliance exists there, then it would never check /usr/lib64/guestfs.
It's a bit wrong, the first step is search supermin.d in all specified directories: /* Step (1). */ r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); if (r == -1) return -1;
if (r == 1) /* Step (2): build supermin appliance. */ return build_supermin_appliance (g, supermin_path, kernel, initrd, appliance);
OK, I see, that is indeed a bug. Can you suggest a patch which fixes this so that directories are searched correctly?
In fact, it's hard for me to understand this is a bug or not, because it's not clear what behavior is expected. The first patch was aimed at bringing consistency between documentation and code. But of course, it doesn't solve the real problem. Changing the priority doesn't change the overall situation, the only difference is the type of appliance through which still someone could accidentally stumble. The fix for this "bug", specifically the search for all appliances in each directory separately, in fact it doesn't fix the real problem, similarly the supermin.d can fall into the new directory and libguestfs can pick up another's appliance.
However I still think that putting all of the appliances into a single directory and expecting libguestfs to know which one you meant it to choose first is the wrong idea.
I agree, to lay certain order in which the libguestfs chooses appliances is astrange idea, but that's how it is now implemented. And it's worth to fix. We can't guarantee that the same directory cannot contain two different appliances. Some distributions can install fixed appliance from packges (http://euscan.gentooexperimental.org/files/gentoo/app-emulation/libguestfs-a...) and here is a possible different situation. The user will have trouble understanding why after installation "package A" is broken "package B". But we can minimally to protect ourselves from unforeseen errors. Now I see two unrelated issues: 1. To search for the supermin appliance using only the directory name, which of course is not enough. If the directory is empty then the libguestfs exits with the following error: supermin: ext2fs_namei: parent directory not found: /usr/lib: File not found by ext2_lookup supermin: failure: ext2fs_namei: parent directory not found libguestfs: error: /usr/bin/supermin exited with error status 1, see debug messages above Where of course the real reason is not visible. So I agree with your proposal to add more files to check, I'll send a patch. 2. If the directory contains two different types of appliances, it is not obvious how the libguestfs will work (without looking at code). This can lead to different breakdowns (mismatch between the version of the daemon, a different set of libraries, etc). Despite the fact that there is support for two different types of appliances at build that could help to solve the issue. In my opinion, if the build is configured with options "--disable-appliance --disable-daemon" that pursued a clear goal to use the fixed appliance. It's hard for me to imagine when it's necessary to build with the included appliance and then to use the fixed appliance, or vice versa. For debugging purposes we can always set environment variable. The solution I introduced in the patch "add enable_appliance flag to specify the appliance build", it's a simple change nevertheless it will allow to solve the issue with unexpected errors.
Rich.
The flag determines the behavior of working with the appliance, and specifically the choice to use the build supermin appliance or the fixed appliance. If the flag is false then the libguestfs will only use the fixed appliance, otherwise the build supermin appliance. This patch is aimed at solving the problem with undefined behavior when searching for the appliance. The libguestfs configured with --enable-appliance will only use the build supermin appliance, with --disable-appliance only use the fixed appliance. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- generator/actions_properties.ml | 23 +++++++++++ lib/appliance.c | 84 +++++++++++++++++++++-------------------- lib/guestfs-internal.h | 1 + lib/handle.c | 23 +++++++++++ m4/guestfs_appliance.m4 | 7 ++++ 5 files changed, 97 insertions(+), 41 deletions(-) diff --git a/generator/actions_properties.ml b/generator/actions_properties.ml index 87144b14f..8f172b517 100644 --- a/generator/actions_properties.ml +++ b/generator/actions_properties.ml @@ -608,6 +608,29 @@ The environment variable C<XDG_RUNTIME_DIR> controls the default value: If C<XDG_RUNTIME_DIR> is set, then that is the default. Else F</tmp> is the default." }; + { defaults with + name = "set_appliance"; added = (1, 37, 11); + style = RErr, [Bool "appliance"], []; + fish_alias = ["appliance"]; config_only = true; + blocking = false; + shortdesc = "set enable appliance flag"; + longdesc = "\ +This flag determines the behavior of working with the appliance, +and specifically the choice to use the build supermin appliance or +the fixed appliance. If the flag is false then the libguestfs will +only use the fixed appliance. + +The default is true if the libguestfs is configured with --enable-appliance, +otherwise false." }; + + { defaults with + name = "get_appliance"; added = (1, 37, 11); + style = RBool "appliance", [], []; + blocking = false; + shortdesc = "get enable appliance flag"; + longdesc = "\ +This returns the enable appliance flag." }; + ] let daemon_functions = [ diff --git a/lib/appliance.c b/lib/appliance.c index f12918573..28e03e92f 100644 --- a/lib/appliance.c +++ b/lib/appliance.c @@ -132,53 +132,55 @@ build_appliance (guestfs_h *g, char **initrd, char **appliance) { - int r; - CLEANUP_FREE char *supermin_path = NULL; - CLEANUP_FREE char *path = NULL; - - /* Step (1). */ - r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); - if (r == -1) - return -1; - - if (r == 1) - /* Step (2): build supermin appliance. */ - return build_supermin_appliance (g, supermin_path, - kernel, initrd, appliance); + if (g->enable_appliance) { + CLEANUP_FREE char *supermin_path = NULL; + /* Step (1). */ + int r = find_path (g, contains_supermin_appliance, NULL, &supermin_path); + if (r == -1) + return -1; - /* Step (3). */ - r = find_path (g, contains_fixed_appliance, NULL, &path); - if (r == -1) - return -1; + if (r == 1) + /* Step (2): build supermin appliance. */ + return build_supermin_appliance (g, supermin_path, + kernel, initrd, appliance); + } else { + CLEANUP_FREE char *path = NULL; + /* Step (3). */ + int r = find_path (g, contains_fixed_appliance, NULL, &path); + if (r == -1) + return -1; - if (r == 1) { - const size_t len = strlen (path); - *kernel = safe_malloc (g, len + 6 /* "kernel" */ + 2); - *initrd = safe_malloc (g, len + 6 /* "initrd" */ + 2); - *appliance = safe_malloc (g, len + 4 /* "root" */ + 2); - sprintf (*kernel, "%s/kernel", path); - sprintf (*initrd, "%s/initrd", path); - sprintf (*appliance, "%s/root", path); - return 0; - } + if (r == 1) { + const size_t len = strlen (path); + *kernel = safe_malloc (g, len + 6 /* "kernel" */ + 2); + *initrd = safe_malloc (g, len + 6 /* "initrd" */ + 2); + *appliance = safe_malloc (g, len + 4 /* "root" */ + 2); + sprintf (*kernel, "%s/kernel", path); + sprintf (*initrd, "%s/initrd", path); + sprintf (*appliance, "%s/root", path); + return 0; + } - /* Step (4). */ - r = find_path (g, contains_old_style_appliance, NULL, &path); - if (r == -1) - return -1; + /* Step (4). */ + r = find_path (g, contains_old_style_appliance, NULL, &path); + if (r == -1) + return -1; - if (r == 1) { - const size_t len = strlen (path); - *kernel = safe_malloc (g, len + strlen (kernel_name) + 2); - *initrd = safe_malloc (g, len + strlen (initrd_name) + 2); - sprintf (*kernel, "%s/%s", path, kernel_name); - sprintf (*initrd, "%s/%s", path, initrd_name); - *appliance = NULL; - return 0; + if (r == 1) { + const size_t len = strlen (path); + *kernel = safe_malloc (g, len + strlen (kernel_name) + 2); + *initrd = safe_malloc (g, len + strlen (initrd_name) + 2); + sprintf (*kernel, "%s/%s", path, kernel_name); + sprintf (*initrd, "%s/%s", path, initrd_name); + *appliance = NULL; + return 0; + } } - error (g, _("cannot find any suitable libguestfs supermin, fixed or old-style appliance on LIBGUESTFS_PATH (search path: %s)"), - g->path); + error (g, _("cannot find any suitable %s (search path: %s)"), + g->enable_appliance ? "libguestfs supermin" : + "fixed or old-style appliance on LIBGUESTFS_PATH", + g->path); return -1; } diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h index 5e9d97c07..2b80f1ab9 100644 --- a/lib/guestfs-internal.h +++ b/lib/guestfs-internal.h @@ -414,6 +414,7 @@ struct guestfs_h { bool selinux; /* selinux enabled? */ bool pgroup; /* Create process group for children? */ bool close_on_exit; /* Is this handle on the atexit list? */ + bool enable_appliance; /* Enable the build supermin appliance */ int smp; /* If > 1, -smp flag passed to hv. */ int memsize; /* Size of RAM (megabytes). */ diff --git a/lib/handle.c b/lib/handle.c index 91f5f755d..b2b18b186 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -205,6 +205,16 @@ parse_environment (guestfs_h *g, guestfs_set_verbose (g, b); } + str = do_getenv (data, "LIBGUESTFS_ENABLE_APPLIANCE"); + if (str) { + b = guestfs_int_is_true (str); + if (b == -1) { + error (g, _("%s=%s: non-boolean value"), "LIBGUESTFS_ENABLE_APPLIANCE", str); + return -1; + } + guestfs_set_appliance (g, b); + } + str = do_getenv (data, "LIBGUESTFS_TMPDIR"); if (str && STRNEQ (str, "")) { if (guestfs_set_tmpdir (g, str) == -1) @@ -927,3 +937,16 @@ guestfs_impl_get_smp (guestfs_h *g) { return g->smp; } + +int +guestfs_impl_set_appliance (guestfs_h *g, int v) +{ + g->enable_appliance = !!v; + return 0; +} + +int +guestfs_impl_get_appliance (guestfs_h *g) +{ + return g->enable_appliance; +} diff --git a/m4/guestfs_appliance.m4 b/m4/guestfs_appliance.m4 index 890b1999c..0e0768656 100644 --- a/m4/guestfs_appliance.m4 +++ b/m4/guestfs_appliance.m4 @@ -28,6 +28,13 @@ AM_CONDITIONAL([ENABLE_APPLIANCE],[test "x$ENABLE_APPLIANCE" = "xyes"]) AC_MSG_RESULT([$ENABLE_APPLIANCE]) AC_SUBST([ENABLE_APPLIANCE]) +LIBGUESTFS_ENABLE_APPLIANC=0 +if test "x$ENABLE_APPLIANCE" = "xyes"; then +LIBGUESTFS_ENABLE_APPLIANCE=1 +fi +AC_DEFINE_UNQUOTED([LIBGUESTFS_ENABLE_APPLIANCE], [$LIBGUESTFS_ENABLE_APPLIANCE], + [Define to 1 or 0, depending on the build configuration.]) + if test "x$enable_daemon" != "xyes" && test "x$ENABLE_APPLIANCE" = "xyes" ; then AC_MSG_FAILURE([conflicting ./configure arguments: if you --disable-daemon then you have to --disable-appliance as well, since the appliance contains -- 2.11.0
On Thu, Apr 27, 2017 at 03:55:10PM +0300, Pavel Butsykin wrote:
The flag determines the behavior of working with the appliance, and specifically the choice to use the build supermin appliance or the fixed appliance. If the flag is false then the libguestfs will only use the fixed appliance, otherwise the build supermin appliance.
This patch is aimed at solving the problem with undefined behavior when searching for the appliance. The libguestfs configured with --enable-appliance will only use the build supermin appliance, with --disable-appliance only use the fixed appliance.
It has to be said I like this approach even less :-/ Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com 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
On 27.04.2017 18:28, Richard W.M. Jones wrote:
On Thu, Apr 27, 2017 at 03:55:10PM +0300, Pavel Butsykin wrote:
The flag determines the behavior of working with the appliance, and specifically the choice to use the build supermin appliance or the fixed appliance. If the flag is false then the libguestfs will only use the fixed appliance, otherwise the build supermin appliance.
This patch is aimed at solving the problem with undefined behavior when searching for the appliance. The libguestfs configured with --enable-appliance will only use the build supermin appliance, with --disable-appliance only use the fixed appliance.
It has to be said I like this approach even less :-/
Any arguments? What exactly is wrong?
Rich.
On Fri, Apr 28, 2017 at 05:03:58PM +0300, Pavel Butsykin wrote:
On 27.04.2017 18:28, Richard W.M. Jones wrote:
On Thu, Apr 27, 2017 at 03:55:10PM +0300, Pavel Butsykin wrote:
The flag determines the behavior of working with the appliance, and specifically the choice to use the build supermin appliance or the fixed appliance. If the flag is false then the libguestfs will only use the fixed appliance, otherwise the build supermin appliance.
This patch is aimed at solving the problem with undefined behavior when searching for the appliance. The libguestfs configured with --enable-appliance will only use the build supermin appliance, with --disable-appliance only use the fixed appliance.
It has to be said I like this approach even less :-/
Any arguments? What exactly is wrong?
Specifically it exposes a whole new API which just serves to adjust some internal issue in libguestfs (which as I said in the other reply is really a bug in libguestfs). Adding new APIs has a high bar because we have to support them forever. I really think the solution here is to fix the path search so it works like you'd expect a path search to work, ie: LIBGUESTFS_PATH=/a:/b:/c means look in /a for any appliance, then look in /b, then look in /c. (Whereas currently it means something like "go round and round in circles looking for different sorts of appliance".) Then change the distro so it doesn't install two appliances into a single location, which only ever worked by random chance (if indeed it ever did work). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 28.04.2017 17:29, Richard W.M. Jones wrote:
On Fri, Apr 28, 2017 at 05:03:58PM +0300, Pavel Butsykin wrote:
On 27.04.2017 18:28, Richard W.M. Jones wrote:
On Thu, Apr 27, 2017 at 03:55:10PM +0300, Pavel Butsykin wrote:
The flag determines the behavior of working with the appliance, and specifically the choice to use the build supermin appliance or the fixed appliance. If the flag is false then the libguestfs will only use the fixed appliance, otherwise the build supermin appliance.
This patch is aimed at solving the problem with undefined behavior when searching for the appliance. The libguestfs configured with --enable-appliance will only use the build supermin appliance, with --disable-appliance only use the fixed appliance.
It has to be said I like this approach even less :-/
Any arguments? What exactly is wrong?
Specifically it exposes a whole new API which just serves to adjust some internal issue in libguestfs (which as I said in the other reply is really a bug in libguestfs). Adding new APIs has a high bar because we have to support them forever.
Thank you for the explanation.
I really think the solution here is to fix the path search so it works like you'd expect a path search to work, ie:
LIBGUESTFS_PATH=/a:/b:/c
means look in /a for any appliance, then look in /b, then look in /c. (Whereas currently it means something like "go round and round in circles looking for different sorts of appliance".)
Then change the distro so it doesn't install two appliances into a single location, which only ever worked by random chance (if indeed it ever did work).
I agree that this will solve my problem, although the error is theoretically possible but in practice this is unrealistic. Given the relative simplicity of the changes, it makes sense to make such a fix, thanks.
Rich.
And also it would be nice if there was a ./configure option to set the default path (at the moment it's hard-coded). That way you could set up the path to search in multiple directories, as appropriate for your downstream distro. 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 28.04.2017 17:31, Richard W.M. Jones wrote:
And also it would be nice if there was a ./configure option to set the default path (at the moment it's hard-coded). That way you could set up the path to search in multiple directories, as appropriate for your downstream distro.
Yes, it's a good point, I will do so.
Rich.
participants (2)
-
Pavel Butsykin -
Richard W.M. Jones