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

[Jan Beulich <jbeulich@xxxxxxxx>]

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