[XEN PATCH 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Hongbo <hehongbo@xxxxxxxx>]

`QEMU_XEN_PATH` will be configured as `qemu-system-i386` with no clue where, if
`--with-system-qemu` is set without giving a path (as matched in the case `yes`
but not `*`). However, the existence of the executable is checked by `access()`,
that will not look for anywhere in $PATH but the current directory. And since it
is possible for `qemu-system-i386` (or any other configured values) to be
executed from PATH later, we'd better find that in PATH and return the full path
for the caller to check against.

Signed-off-by: Hongbo <hehongbo@xxxxxxxx>

---

This patch is from the maintenance team of the Xen Project Hypervisor at NixOS.
We encountered this and thought it was an edge case, and came up with this while
maintaining the package and module of the Hypervisor at NixOS.

According to `xen.git/tools/configure.ac`, `QEMU_XEN_PATH` will be configured as
`qemu-system-i386` (relative) if `--with-system-qemu` is set (as `yes`) but
without an absolute path. However, it will execute `qemu-system-i386` from the
`PATH` only if a file is called `qemu-system-i386` in the *current directory*.
That is because the existence of the device model executable file, in this case
`qemu-system-i386`, is checked via `access()` without concatenating it with
current PATHs. And `access()` is not tailored for executables, it will not
search for the PATHs for us.

See `libxl__domain_build_info_setdefault()` at
`xen.git/tools/libs/light/libxl_create.c`. It reads `dm` from
`libxl__domain_device_model()` and then uses `access()` on it. If that fails, it
will modify the `device_model_version` to qemu-traditional. Then, in
`libxl__spawn_local_dm()` at `xen.git/tools/lib/light/libxl_dm.c`, it reads from
`libxl__domain_device_model()` again, and `access()` is used again to detect the
file's existence. In my investigations, if I comment out these 2 existence
checks then it will run `qemu-system-i386` from the current PATH without issues.
I guess if it's not blocked by those 2 checks, it will finally reach
`libxl__exec()`. Then, inside the `libxl__exec()` the device model executable
will be executed by `execvp()`, which can certainly call the executable from
both absolute paths and current PATHs.

Since the device model executable will be checked twice, both of which will call
`libxl__domain_device_model()` to get its location, I think the preferred
solution would be patching the `libxl__domain_device_model()` function itself to
tell if we're referring to an executable in PATHs, and resolve to the full path
of it for the caller to check against.

It's indeed an edge case. But why would we need this? Because in Nix (the
package manager) and NixOS, we use Nix expressions to declare dependencies on
the dependents, and we ran into the issue of circular dependency - to build
QEMU with Xen support, we should give the Xen header and libraries into the
building process of QEMU, that makes Xen (`pkgs.xen`) a dependency of QEMU
(`pkgs.qemu_xen`), which prevents us from using `pkgs.qemu_xen` in the building
process of Xen, and in `--with-system-qemu=` argument in particular. It is very
different compared to those distros and package managers that follow the
Filesystem Hierarchy Standard (FHS), in which Xen can be built with
`--with-system-qemu=` points to a non-existent FHS location of
`qemu-system-i386`, and then use these Xen libraries from the artifacts to build
QEMU afterward. So we decide to build Xen with `--with-system-qemu` but not
including an executable path, taking advantage of the fact that `QEMU_XEN_PATH`
can be configured as a relative `qemu-system-i386` when omitted, as declared as
the `yes` case in `xen.git/tools/configure.ac`, and that results in we finding
the aforementioned "current directory" issue, and submitting this patch.

In the patch, I'm using the existence of slash (`/`) to tell if `QEMU_XEN_PATH`
is relative, and begin to search in PATH if it is. I'm sort of iffy on this,
would it make more sense if we do this on inputs starting with a slash instead?
And should we notify the user if it's not found anywhere in the PATH thus
proceeding with the value configured in `QEMU_XEN_PATH` as-is?

Let me know if it's appropriate and if further changes are needed.

Best regards,
Hongbo

---
CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
CC: Juergen Gross <jgross@xxxxxxxx>
---
 tools/libs/light/libxl_dm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 1f2f5bd97a..db05f20a5b 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -332,7 +332,39 @@ const char *libxl__domain_device_model(libxl__gc *gc,
             dm = libxl__abs_path(gc, "qemu-dm", libxl__private_bindir_path());
             break;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            dm = qemu_xen_path(gc);
+            const char *configured_dm = qemu_xen_path(gc);
+            if (strchr(configured_dm, '/'))
+                dm = libxl__strdup(gc, configured_dm);
+            else
+            {
+                const char *path_env = getenv("PATH");
+                if (!path_env)
+                    dm = libxl__strdup(gc, configured_dm);
+                else
+                {
+                    char *path_dup = libxl__strdup(gc, path_env);
+                    char *saveptr;
+
+                    char *path = strtok_r(path_dup, ":", &saveptr);
+                    char fullpath[PATH_MAX];
+                    bool dm_found = false;
+                    while (path)
+                    {
+                        snprintf(fullpath, sizeof(fullpath), "%s/%s", path,
+                                 configured_dm);
+                        if (access(fullpath, X_OK) == 0)
+                        {
+                            dm = libxl__strdup(gc, fullpath);
+                            dm_found = true;
+                            break;
+                        }
+                        path = strtok_r(NULL, ":", &saveptr);
+                    }
+
+                    if (!dm_found)
+                        dm = libxl__strdup(gc, configured_dm);
+                }
+            }
             break;
         default:
             LOG(ERROR, "invalid device model version %d",
--
2.39.5 (Apple Git-154)