Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs

[Fabio Fantoni <fabio.fantoni@heliman.it>]

Il 18/02/2013 11:06, Ian Campbell ha scritto:
> On Fri, 2013-02-15 at 21:54 +0000, Fabio Fantoni wrote:
>> Il 15/02/2013 16:48, Ian Jackson ha scritto:
>>> fantonifabio@tiscali.it writes ("[Xen-devel] [PATCH] tools/libxl: Added vga parameter for hvm domUs"):
>>>> From: Fabio Fantoni <fabio.fantoni@heliman.it>
>>>>
>>>> Usage:
>>>>     vga="stdvga"|"cirrus"
>>>>
>>>> - Default option is cirrus.
>>>> - Prints error and exit if unknown value is passed.
>>>> - stdvga parameter is now deprecated.
>>>> - Updated xl.cfg man.
>>>>
>>>> Required patch: Improve videoram setting v5
>>>> Is prerequisite for patch: Add qxl support v9
>>> The code looks good to me, but there are a couple of formatting
>>> problems.  Your wrapping of the longer lines is odd (particularly, you
>>> fail to indent the continuations), and there's a spurious space after
>>> xlu_cfg_get_string.  Can you please repost following the style used
>>> elsewhere ?
>>>
>>> Thanks,
>>> Ian.
>> Sorry.
>> Can you explain me the error of indentation did I do please?
> The spurious space after xlu_cfg_get_string, e.g.
> 		xlu_cfg_get_string (config,
> should have been
> 		xlu_cfg_get_string(config,
>
> However, I count 44 instances with the space and 3 without in this file,
> so I think you can be forgiven. This extra space has been there from day
> one...
>
> The more important one IMHO is:
>
> +            if (!strcmp(buf, "stdvga")) {
> +                b_info->u.hvm.vga.kind
> +                = LIBXL_VGA_INTERFACE_TYPE_STD;
>
> I noticed this when I reviewed but when I looked at the file I concluded
> you were just copying a prevailing style so I let it slide. Looking
> again I think I was looking at the file with this patch applied, which
> makes that logic rather circular ;-)
>
> It seems like you were trying to avoid going over the 80 column limit,
> although in this case it doesn't seem like the line would actually be
> that long. If wrapping is required then it would be more normal to
> indent as:
>
> +            if (!strcmp(buf, "stdvga")) {
> +                b_info->u.hvm.vga.kind
> +                    = LIBXL_VGA_INTERFACE_TYPE_STD;
>
> (i.e indent the wrapped line by one level). However given that all these
> things fit on one line using at most 72 columns:
>
> 8<------
> xl: fix line wrapping oddness
>
> These lines are strangely wrapped and in any case fit within 80 columns
> when unwrapped.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 003b129..a80cda6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1476,14 +1476,11 @@ skip_vfb:
>       if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
>           if (!xlu_cfg_get_string (config, "vga", &buf, 0)) {
>               if (!strcmp(buf, "stdvga")) {
> -                b_info->u.hvm.vga.kind
> -                = LIBXL_VGA_INTERFACE_TYPE_STD;
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
>               } else if (!strcmp(buf, "cirrus")) {
> -                b_info->u.hvm.vga.kind
> -                = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>               } else {
> -                fprintf(stderr,
> -                "Unknown vga \"%s\" specified\n", buf);
> +                fprintf(stderr, "Unknown vga \"%s\" specified\n", buf);
>                   exit(1);
>               }
>           } else if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
>
>
>
>
Sorry for my coding style errors, I saw this mail only after posting v10 
of qxl patch, I'll do v11 after posting your coding style fix.