Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-03-17 at 15:46 +0800, Chen, Tiejun wrote:
> >>> If I remember the context correctly this is in the autodetect case,
> >>> so I think shouldn't mention IGD. Something like "Unable to detect
> >>> graphics passthru kind, please set gfx_passthru_kind. See xl.cfg(5)
> >>> for more
> >>
> >> s/gfx_passthru_kind/gfx_passthru, right? Because actually we always get
> >> 'gfx_passthru_kind' from 'gfx_passthru'.
> >
> > I think you have it backwards.
> >
> > In the case here gfx_passthru=1 has been set by the user, but
> > gfx_passthru_kind=DEFAULT. So libxl has tried to autodetect but it has
> > failed.
> >
> > So if the user wants to make progress they should set gfx_passthru_kind
> > to whatever type of passthrough they were trying to do.
> 
> Looks you're saying 'gfx_passthru_kind' shouldn't reply on 
> 'gfx_passthru', so 'gfx_passthru_kind' can override that previous value 
> parsed from 'gfx_passthru', right?

I think perhaps the confusion arises because libxl and xl differ here. I
was commenting on some libxl code, which has two fields, whereas you are
talking about the xl cfg level, which only has one, which can be parsed
in two ways. So I think I've probably been adding lots of confusion
here, sorry.

I think the _libxl_ message needs to be just "Unable to detect graphics
passthru kind". i.e. it can't/shouldn't reference anything to do with xl
config options etc (which would make no sense if libvirt was being
used).

That's not very user friendly though, so you may want to consider adding
a new specific error code for this case and returning it here, such that
xl or libvirt can then give a more comprehensible error message.

> So looks the whole policy should be something like this,
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5c40e84..5518759 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1953,8 +1953,27 @@ skip_vfb:
>           xlu_cfg_replace_string (config, "spice_streaming_video",
>                                   &b_info->u.hvm.spice.streaming_video, 0);
>           xlu_cfg_get_defbool(config, "nographic", 
> &b_info->u.hvm.nographic, 0);
> -        xlu_cfg_get_defbool(config, "gfx_passthru",
> -                            &b_info->u.hvm.gfx_passthru, 0);
> +        if (!xlu_cfg_get_long(config, "gfx_passthru", &l, 1)) {
> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, l);
> +        } else if (!xlu_cfg_get_string(config, "gfx_passthru", &buf, 0)) {
> +            if (libxl_gfx_passthru_kind_from_string(buf,
> + 
> &b_info->u.hvm.gfx_passthru_kind)) {
> +                fprintf(stderr,
> +                        "ERROR: invalid value \"%s\" for 
> \"gfx_passthru\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +            libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
> +        }

Up to here is fine.

> +        if (!xlu_cfg_get_string(config, "gfx_passthru_kind", &buf, 0)) {
> +            if (libxl_gfx_passthru_kind_from_string(buf,
> + 
> &b_info->u.hvm.gfx_passthru_kind)) {
> +                fprintf(stderr,
> +                        "ERROR: invalid value \"%s\" for 
> \"gfx_passthru_kind\"\n",
> +                        buf);
> +                exit (1);
> +            }
> +        }

I don't think you need this bit.