Re: [Xen-devel] [PATCH]libxl:refactor the code of stdvga option support

[ZhouPeng <zpengxen@xxxxxxxxx>]

On Tue, May 29, 2012 at 5:17 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Mon, 2012-05-28 at 09:52 +0100, ZhouPeng wrote:
>> refactor the code of stdvga option support.
>>
>> Be ready to add and describe new vga interface
>
> Are you proposing this for 4.2, which is currently in feature freeze?
>
> I'd be inclined to accept this particular for 4.2 due to the API change,
for the upstream-xen and upstream-xen-qemu
> but I'm curious what the "new vga interface" is going to be.
QXL
>
>> Signed-off-by: Zhou Peng <ailvpeng25@xxxxxxxxx>
>>
>> diff -r 592d15bd4d5e tools/libxl/libxl_types.idl
>> --- a/tools/libxl/libxl_types.idl     Fri May 18 16:19:21 2012 +0100
>> +++ b/tools/libxl/libxl_types.idl     Mon May 28 16:10:02 2012 +0800
>> @@ -125,9 +125,20 @@ libxl_shutdown_reason = Enumeration("shu
>>      (4, "watchdog"),
>>      ])
>>
>> +libxl_vga_interface_type = Enumeration("vga_interface_type", [
>> +    (0, "DEFAULT"),
>
> "DEFAULT" here just means "whatever qemu gives you if you don't say
> otherwise"?
Yes,
It keep the same with the current libxl behavior (trans nothing for
vga in the qemu cmd).
> What actually is that?
Cirrus will be selected by qemu in face.
> I think we'd be better off having it
> as an explicit option too and using setdefault to convert DEFAULT->that.

Ok.
>
>> +    (1, "STD"),
>> +    ])
>> +
>>  #
>>  # Complex libxl types
>>  #
>> +
>> +libxl_vga_interface_info = Struct("vga_interface_info", [
>> +    ("type",    libxl_vga_interface_type),
>> +    ("vramkb",  MemKB),
>
> I can't see "vramkb" being used anywhere. Did you mean to include it in
> the followup "new vga interface" patch?

Yes.
By qxl or other vga in the future.
>
>> +    ])
>> +
>>  libxl_vnc_info = Struct("vnc_info", [
>>      ("enable",        libxl_defbool),
>>      # "address:port" that should be listened on
>> @@ -286,7 +297,7 @@ libxl_domain_build_info = Struct("domain
>>                                         ("nested_hvm",       libxl_defbool),
>>                                         ("incr_generationid",libxl_defbool),
>>                                         ("nographic",        libxl_defbool),
>> -                                       ("stdvga",           libxl_defbool),
>> +                                       ("vga",
>> libxl_vga_interface_info),
>>                                         ("vnc",              libxl_vnc_info),
>>                                         # keyboard layout, default is
>> en-us keyboard
>>                                         ("keymap",           string),
>
> Looks like this bit is whitespace damaged.
>
> http://wiki.xen.org/wiki/Submitting_Xen_Patches has some hints on using
> the mercurial patchbomb tool to avoid this and also a link to
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/email-clients.txt;hb=HEAD
> which gives advice on using various mailclients to manually send patches
> without mangling them.
>> diff -r 592d15bd4d5e tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c        Fri May 18 16:19:21 2012 +0100
>> +++ b/tools/libxl/xl_cmdimpl.c        Mon May 28 16:10:02 2012 +0800
>> @@ -1256,7 +1256,12 @@ skip_vfb:
>>  #undef parse_extra_args
>>
>>      if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>> -        xlu_cfg_get_defbool(config, "stdvga", &b_info->u.hvm.stdvga, 0);
>> +        libxl_defbool vga;
>> +        b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_DEFAULT;
>> +        if (!xlu_cfg_get_defbool(config, "stdvga", &vga, 0))
>> +            if (libxl_defbool_val(vga))
>
> I don't think you need to launder this through a defbool, since it is no
> longer one at the libxl interface. Use xlu_cfg_get_long into &l and
> then
>        if (l)

Ok.
>                b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_STD;
>
>> +                b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_STD;
>> +
>>          xlu_cfg_get_defbool(config, "vnc", &b_info->u.hvm.vnc.enable, 0);
>>          xlu_cfg_replace_string (config, "vnclisten",
>>                                  &b_info->u.hvm.vnc.listen, 0);
>> diff -r 592d15bd4d5e tools/libxl/xl_sxp.c
>> --- a/tools/libxl/xl_sxp.c    Fri May 18 16:19:21 2012 +0100
>> +++ b/tools/libxl/xl_sxp.c    Mon May 28 16:10:02 2012 +0800
>> @@ -110,8 +110,10 @@ void printf_info_sexp(int domid, libxl_d
>>                 libxl_defbool_to_string(b_info->u.hvm.nested_hvm));
>>          printf("\t\t\t(no_incr_generationid %s)\n",
>>                 libxl_defbool_to_string(b_info->u.hvm.incr_generationid));
>> -        printf("\t\t\t(stdvga %s)\n",
>> -               libxl_defbool_to_string(b_info->u.hvm.stdvga));
>> +        libxl_defbool stdvga;
>> +        libxl_defbool_set(&stdvga,
>> +                   b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_STD);
>> +        printf("\t\t\t(stdvga %s)\n", libxl_defbool_to_string(stdvga));
>
> Likewise I think this is just
>        printf("\t\t\t(stdvga %s)\n",
>               b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_STD) ?
>               True" : "False");
>
>>          printf("\t\t\t(vnc %s)\n",
>>                 libxl_defbool_to_string(b_info->u.hvm.vnc.enable));
>>          printf("\t\t\t(vnclisten %s)\n", b_info->u.hvm.vnc.listen);
>>

Ok.
>
>



-- 
Zhou Peng

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel