On 30.03.2025 18:03, Hongbo wrote:
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -331,9 +331,43 @@ const char *libxl__domain_device_model(libxl__gc *gc,
> case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> dm = libxl__abs_path(gc, "qemu-dm",
> libxl__private_bindir_path());
> break;
> - case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> - dm = qemu_xen_path(gc);
> + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: {
> + const char *configured_dm = qemu_xen_path(gc);
What about this returning a filename with a path, just not an absolute one?
$PATH
shouldn't be searched in such a case, should it?
> + if (configured_dm[0] == '/')
> + {
> + dm = configured_dm;
> + }
Why is this and ...
> + else
> + {
> + const char *path_env = getenv("PATH");
> + if (!path_env)
> + {
> + dm = configured_dm;
> + }
... this needed, when at the bottom dm is defaulted to dm_configured anyway?
You could set dm to dm_configured uniformly up front. Furthermore there's
then no real need for dm_configured then.
> + else
> + {
> + char *path_dup = libxl__strdup(gc, path_env);
> + char *saveptr;
> +
> + char *path = strtok_r(path_dup, ":", &saveptr);
Main reason I'm replying here is this one though, where CI found gcc to
object:
libxl_dm.c: In function 'libxl__domain_device_model':
libxl_dm.c:356:31: error: 'saveptr' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
char *candidate = libxl__abs_path(gc, configured_dm,
path);
^
cc1: all warnings being treated as errors
The compiler can't know that path_dup is guaranteed non-NULL. Hence, if it
can see (part of) the implementation of strtok_r(), it would observe that
it's possible that the continuation-invocation path is taken, where
saveptr necessarily is consumed.
Taking together all the issues I think I'll revert this, for you to make
another attempt. It may have been a mistake to commit such a change anyway;
I might better have left that to Anthony.
Jan
