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

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/3/19 18:44, Ian Campbell wrote:
> 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.
>

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");
+            default:
+                return NULL;
+            }
+        }
+
          flexarray_append(dm_args, machinearg);
          for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; 
i++)
              flexarray_append(dm_args, b_info->extra_hvm[i]);


Thanks
Tiejun