[PATCH 0/2] lib: Allow db_dump package to be a weak dependency.
--- lib/inspect-apps.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c index f0cf16b38..020a3332c 100644 --- a/lib/inspect-apps.c +++ b/lib/inspect-apps.c @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g, const char *root) if (STREQ (type, "linux") || STREQ (type, "hurd")) { if (STREQ (package_format, "rpm")) { #ifdef DB_DUMP - ret = list_applications_rpm (g, root); - if (ret == NULL) - return NULL; + /* Allow db_dump to be missing at runtime so that it can be + * installed as a weak dependency. + */ + if (system (DB_DUMP " -V >/dev/null 2>&1") == 0) { + ret = list_applications_rpm (g, root); + if (ret == NULL) + return NULL; + } #endif } else if (STREQ (package_format, "deb")) { -- 2.13.2
On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones wrote:
--- lib/inspect-apps.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c index f0cf16b38..020a3332c 100644 --- a/lib/inspect-apps.c +++ b/lib/inspect-apps.c @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g, const char *root) if (STREQ (type, "linux") || STREQ (type, "hurd")) { if (STREQ (package_format, "rpm")) { #ifdef DB_DUMP - ret = list_applications_rpm (g, root); - if (ret == NULL) - return NULL; + /* Allow db_dump to be missing at runtime so that it can be + * installed as a weak dependency. + */ + if (system (DB_DUMP " -V >/dev/null 2>&1") == 0) { + ret = list_applications_rpm (g, root); + if (ret == NULL) + return NULL; + } #endif
This approach has few issues: a) DB_DUMP is not escaped, so a path with special characters will play badly with shell b) `DB_DUMP -V` is executed every time RPM packages are extracted c) there is no feedback to the users (not even a debug()) that DB_DUMP is not installed, and thus information on the installed RPM packages are not extracted d) the same issue happens for the tools used to extract icons (netpbm, and icoutils) I still do think my approach [1] is better in this regard, since it already copes with all the points above. Why should be optional tools searched at build time, hardcoded, and then still need to be checked at runtime? The concern that there is no way to know which tools are used is sort of moot, since the internal command API (in the library) logs program and arguments. [1] https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html -- Pino Toscano
On Fri, Oct 06, 2017 at 10:09:03AM +0200, Pino Toscano wrote:
On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones wrote:
--- lib/inspect-apps.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c index f0cf16b38..020a3332c 100644 --- a/lib/inspect-apps.c +++ b/lib/inspect-apps.c @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g, const char *root) if (STREQ (type, "linux") || STREQ (type, "hurd")) { if (STREQ (package_format, "rpm")) { #ifdef DB_DUMP - ret = list_applications_rpm (g, root); - if (ret == NULL) - return NULL; + /* Allow db_dump to be missing at runtime so that it can be + * installed as a weak dependency. + */ + if (system (DB_DUMP " -V >/dev/null 2>&1") == 0) { + ret = list_applications_rpm (g, root); + if (ret == NULL) + return NULL; + } #endif
This approach has few issues: a) DB_DUMP is not escaped, so a path with special characters will play badly with shell b) `DB_DUMP -V` is executed every time RPM packages are extracted c) there is no feedback to the users (not even a debug()) that DB_DUMP is not installed, and thus information on the installed RPM packages are not extracted d) the same issue happens for the tools used to extract icons (netpbm, and icoutils)
I still do think my approach [1] is better in this regard, since it already copes with all the points above.
Why should be optional tools searched at build time, hardcoded, and then still need to be checked at runtime? The concern that there is no way to know which tools are used is sort of moot, since the internal command API (in the library) logs program and arguments.
[1] https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html
I mostly agree with all of what you say, EXCEPT that in the past we've had cases where people have installed broken binaries on /usr/local/bin and then reported bugs against libguestfs. Therefore at configure time we need to store the actual path of the binaries we want to use (and the binaries that the packager tested against) and we shouldn't search $PATH at runtime. (qemu-img is also problematic here, see recent thread on this list) So some generic mechanism is fine, and looking for binaries at runtime is also fine, but not one which is searching $PATH. 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, 6 October 2017 12:23:00 CEST Richard W.M. Jones wrote:
On Fri, Oct 06, 2017 at 10:09:03AM +0200, Pino Toscano wrote:
On Thursday, 5 October 2017 15:41:31 CEST Richard W.M. Jones wrote:
--- lib/inspect-apps.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/inspect-apps.c b/lib/inspect-apps.c index f0cf16b38..020a3332c 100644 --- a/lib/inspect-apps.c +++ b/lib/inspect-apps.c @@ -122,9 +122,14 @@ guestfs_impl_inspect_list_applications2 (guestfs_h *g, const char *root) if (STREQ (type, "linux") || STREQ (type, "hurd")) { if (STREQ (package_format, "rpm")) { #ifdef DB_DUMP - ret = list_applications_rpm (g, root); - if (ret == NULL) - return NULL; + /* Allow db_dump to be missing at runtime so that it can be + * installed as a weak dependency. + */ + if (system (DB_DUMP " -V >/dev/null 2>&1") == 0) { + ret = list_applications_rpm (g, root); + if (ret == NULL) + return NULL; + } #endif
This approach has few issues: a) DB_DUMP is not escaped, so a path with special characters will play badly with shell b) `DB_DUMP -V` is executed every time RPM packages are extracted c) there is no feedback to the users (not even a debug()) that DB_DUMP is not installed, and thus information on the installed RPM packages are not extracted d) the same issue happens for the tools used to extract icons (netpbm, and icoutils)
I still do think my approach [1] is better in this regard, since it already copes with all the points above.
Why should be optional tools searched at build time, hardcoded, and then still need to be checked at runtime? The concern that there is no way to know which tools are used is sort of moot, since the internal command API (in the library) logs program and arguments.
[1] https://www.redhat.com/archives/libguestfs/2017-February/msg00016.html
I mostly agree with all of what you say, EXCEPT that in the past we've had cases where people have installed broken binaries on /usr/local/bin and then reported bugs against libguestfs.
I would consider this situation broken anyway, and as mentioned above it will be spotted by looking at the verbose logs (which show the executable run, and with my changes they are passed as absolute).
Therefore at configure time we need to store the actual path of the binaries we want to use (and the binaries that the packager tested against) and we shouldn't search $PATH at runtime.
The current system though will not prevent any of those mistakes if the user builds libguestfs after installing the binaries in /usr/local (or anywhere else in $PATH): the configure of libguestfs will pick them, and thus back at the issue. Instead, looking them only at runtime means the user can just cleanup the broken binaries, with no need to rebuild libguestfs later.
So some generic mechanism is fine, and looking for binaries at runtime is also fine, but not one which is searching $PATH.
If $PATH has broken stuff, then it is a bigger problem than just libguestfs. Do not get me wrong: I understand the concern here, but IMHO it should be better to just rely on the good status of the system (as it is supposed to be), rather than "hacking around it". -- Pino Toscano
On Fri, Oct 06, 2017 at 03:11:40PM +0200, Pino Toscano wrote:
On Friday, 6 October 2017 12:23:00 CEST Richard W.M. Jones wrote:
I mostly agree with all of what you say, EXCEPT that in the past we've had cases where people have installed broken binaries on /usr/local/bin and then reported bugs against libguestfs.
I would consider this situation broken anyway, and as mentioned above it will be spotted by looking at the verbose logs (which show the executable run, and with my changes they are passed as absolute).
Certainly agreed the situation is broken when someone puts a broken binary in /usr/local.
Therefore at configure time we need to store the actual path of the binaries we want to use (and the binaries that the packager tested against) and we shouldn't search $PATH at runtime.
The current system though will not prevent any of those mistakes if the user builds libguestfs after installing the binaries in /usr/local (or anywhere else in $PATH): the configure of libguestfs will pick them, and thus back at the issue.
In that situation, the tests will run against those binaries from /usr/local, and libguestfs will run against those same binaries. That's fine - we have proved those binaries aren't broken! But really I'm talking about the majority of consumers who get libguestfs from their distro.
Instead, looking them only at runtime means the user can just cleanup the broken binaries, with no need to rebuild libguestfs later.
So some generic mechanism is fine, and looking for binaries at runtime is also fine, but not one which is searching $PATH.
If $PATH has broken stuff, then it is a bigger problem than just libguestfs. Do not get me wrong: I understand the concern here, but IMHO it should be better to just rely on the good status of the system (as it is supposed to be), rather than "hacking around it".
I really don't think this is hacking around anything. It's addressing a problem which has come up in reality. It's not hacking because the distro packager has built (and one hopes checked) that libguestfs works against a certain mix of binaries, namely those provided under /usr/bin by the distro. The distro is responsible for those binaries, but not the ones under /usr/local/bin. I think this is about the integrity of what we ship to the majority of users. FWIW I posted a v2 of this patch series (actually, before I saw this email, because the mailing list is incredibly slow this week for some reason). It attempts to address the problem in a way which is simple yet allows weak dependencies. Also I posted another patch to address the qemu-img situation which you mentioned back in your posting in February. 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
This function returns -1 if things like fork(2) fail, so it is right to return an error here. If the PBMTEXT command fails then that's a NOT_FOUND case. --- lib/inspect-icon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/inspect-icon.c b/lib/inspect-icon.c index 533e9fb4b..0edd3d1b0 100644 --- a/lib/inspect-icon.c +++ b/lib/inspect-icon.c @@ -400,7 +400,7 @@ icon_cirros (guestfs_h *g, size_t *size_r) guestfs_int_cmd_add_string_quoted (cmd, pngfile); r = guestfs_int_cmd_run (cmd); if (r == -1) - return NOT_FOUND; + return NULL; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) return NOT_FOUND; -- 2.13.2
On Thursday, 5 October 2017 15:41:32 CEST Richard W.M. Jones wrote:
This function returns -1 if things like fork(2) fail, so it is right to return an error here. If the PBMTEXT command fails then that's a NOT_FOUND case. --- lib/inspect-icon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/inspect-icon.c b/lib/inspect-icon.c index 533e9fb4b..0edd3d1b0 100644 --- a/lib/inspect-icon.c +++ b/lib/inspect-icon.c @@ -400,7 +400,7 @@ icon_cirros (guestfs_h *g, size_t *size_r) guestfs_int_cmd_add_string_quoted (cmd, pngfile); r = guestfs_int_cmd_run (cmd); if (r == -1) - return NOT_FOUND; + return NULL; if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) return NOT_FOUND;
LGTM (it looks like this issue dates back to when the code was introduced, even before CLEANUP_* & co). -- Pino Toscano
participants (2)
-
Pino Toscano -
Richard W.M. Jones