Re: [v2][PATCH 2/2] libxl: introduce gfx_passthru_kind

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

On Fri, 2015-03-20 at 09:04 +0800, Chen, Tiejun wrote:
> Refactor again,
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 8599a6a..05c8916 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 enum libxl_gfx_passthru_kind
> +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 LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
> +}
> +
>   static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                           const char *dm, int guest_domid,
>                                           const libxl_domain_config 
> *guest_config,
> @@ -757,6 +771,21 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                               machinearg, max_ram_below_4g);
>               }
>           }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            enum libxl_gfx_passthru_kind gfx_passthru_kind =
> +                            libxl__detect_gfx_passthru_kind(gc, 
> guest_config);
> +            switch (gfx_passthru_kind) {
> +            case LIBXL_GFX_PASSTHRU_KIND_IGD:
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +                break;
> +            case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
> +                LOG(ERROR, "unable to detect required gfx_passthru_kind");

In this case you will now have logged twice. I'd suggest logging only
here and not in the helper.

> +            default:

And this case is subtly different to LIBXL_GFX_PASSTHRU_KIND_DEFAULT
since it would indicate some sort of corruption. So, I would drop the
logging on failure in libxl__detect_gfx_passthru_kind and here do:
           case LIBXL_GFX_PASSTHRU_KIND_DEFAULT:
               LOG(ERROR, "unable to detect required gfx_passthru_kind");
               return NULL;
           default:
               LOG(ERROR, "invalid value for gfx_passthru_kind");
               return NULL;

Ian