[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind
From: |
Ian Campbell |
Subject: |
Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind |
Date: |
Thu, 19 Mar 2015 10:44:20 +0000 |
On Thu, 2015-03-19 at 10:07 +0800, Chen, Tiejun wrote:
> > This duplicates the code from above. I think this would be best done as:
> >
> > static int libxl__detect_gfx_passthru_kind(libxl__gc *gc, guest_config)
> > {
> > if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> > return 0;
> >
> > if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> > b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
> > return 0;
> > }
> >
> > LOG(ERROR, "Unable to detect graphics passthru kind");
> > return 1;
> > }
> >
> > Then for the code in libxl__build_device_model_args_new:
> >
> > if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> > if (!libxl__detect_gfx_passthru_kind(gc, guest_config))
> > return NULL
> > switch (b_info->u.hvm.gfx_passthru_kind) {
> > case LIBXL_GFX_PASSTHRU_KIND_IGD:
> > machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> > break;
> > default:
> > LOG(ERROR, "unknown gfx_passthru_kind\n");
> > return NULL;
> > }
> > }
> >
> > That is, a helper to try and autodetect kind if it is default and then a
> > single switch entry for each kind.
> >
> >> + default:
> >> + LOG(WARN, "gfx_passthru_kind is invalid so ignored.\n");
> >
> > Please return an error here, as I've shown above.
>
> Looks good and thanks, but here 'guest_config' is a const so we
> shouldn't/can't reset b_info->u.hvm.gfx_passthru_kind like this,
>
> b_info->u.hvm.gfx_passthru_kind = LIBXL_GFX_PASSTHRU_KIND_IGD;
>
> So I tried to refactor a little bit to follow up yours,
>
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..605b17c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -409,6 +409,23 @@ static char *dm_spice_options(libxl__gc *gc,
> return opt;
> }
>
> +static int
> +libxl__detect_gfx_passthru_kind(libxl__gc *gc,
> + const libxl_domain_config *guest_config)
> +{
> + const libxl_domain_build_info *b_info = &guest_config->b_info;
> +
> + if (b_info->u.hvm.gfx_passthru_kind != LIBXL_GFX_PASSTHRU_KIND_DEFAULT)
> + return b_info->u.hvm.gfx_passthru_kind;
> +
> + if (libxl__is_igd_vga_passthru(gc, guest_config)) {
> + return LIBXL_GFX_PASSTHRU_KIND_IGD;
> + }
> +
> + LOG(ERROR, "Unable to detect graphics passthru kind");
> + return -1;
I think you can make this function return enum
libxl_gfx_passthrough_kind and then return
LIBXL_GFX_PASSTHRU_KIND_DEFAULT in this case.
> +}
> +
> static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> const char *dm, int guest_domid,
> const libxl_domain_config
> *guest_config,
> @@ -427,7 +444,7 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
> const char *keymap = dm_keymap(guest_config);
> char *machinearg;
> flexarray_t *dm_args;
> - int i, connection, devid;
> + int i, connection, devid, gfx_passthru_kind;
Please declare this in the smallest necessary scope...
[...]
> + if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> + gfx_passthru_kind = libxl__detect_gfx_passthru_kind(gc,
i.e. here.
> +
> guest_config);
> + switch (gfx_passthru_kind) {
> + case LIBXL_GFX_PASSTHRU_KIND_IGD:
> + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> + break;
> + default:
With the suggestion to return KIND_DEFAULT if detection fails then I
think an extra case should be added:
case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
LOG(ERROR, "unable to detect required
gfx_passthru_kind");
return NULL;
> + LOG(ERROR, "unknown gfx_passthru_kind\n");
I think LOG is supposed to not include the final \n.
Ian.
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, (continued)
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/12
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/12
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/13
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/15
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/16
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/17
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/17
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/18
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind,
Ian Campbell <=
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/19
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/20
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/20
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Ian Campbell, 2015/03/20
- Re: [Qemu-devel] [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind, Chen, Tiejun, 2015/03/20
[Qemu-devel] [v2][PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru, Tiejun Chen, 2015/03/10