|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute
On Mon, Dec 23, 2024 at 11:17:53AM +0800, Hongbo wrote:
> `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.
Thanks. Sounds good, and also the feature seems half-way there already
with the default to a binary name and the use of execvp. The patch looks
mostly fine, I still have a few comments.
> 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?
Well, on one hand, that's how execvp() works, if there's a '/' in the
path, search in $PATH is skipped. But that would be a buggy scenario
because when you reboot a guest, the working directory isn't the same
anymore, at least with `xl`. So if `pwd`/dir/qemu-dm exist when you
execute `xl create`, but then run `reboot` in a guest, there's an "xl
daemon" that would try to reboot the guest with /dir/qemu-dm because
`pwd` would be just /.
So I think checking that QEMU_XEN_PATH is absolute would be enough here.
> 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);
Here, `clang` complained about that new variable, it expect an
expression. Can you put the "case" in a block {} ?
> + if (strchr(configured_dm, '/'))
As discuste before, just checking for absolute path should be good
enough here.
> + dm = libxl__strdup(gc, configured_dm);
The function used to return the value from qemu_xen_path(gc), so you can
keep returning `configured_dm`, no need for an strdup, here or the two
other strdup(configured_dm) below.
Also as the `else` part have a block {}, it would be better to have a
block {} for the "true" part of the `if` as well.
> + 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",
Thanks,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |