From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fabio Fantoni Subject: Re: [PATCH] tools/libxl: Added vga parameter for hvm domUs Date: Mon, 18 Feb 2013 11:51:10 +0100 Message-ID: <5122079E.1090600@heliman.it> References: <1360580242-4972-1-git-send-email-fantonifabio@tiscali.it> <20766.22732.806518.892544@mariner.uk.xensource.com> <511EAEA9.5040109@heliman.it> <1361182012.31407.122.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1361182012.31407.122.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" , Ian Jackson , Stefano Stabellini , "fantonifabio@tiscali.it" List-Id: xen-devel@lists.xenproject.org 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 >>>> >>>> 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 > > 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.