On Fri, 2013-02-15 at 21:54 +0000, Fabio Fantoni wrote:
Il 15/02/2013 16:48, Ian Jackson ha scritto:
fantonifabio@xxxxxxxxxx writes ("[Xen-devel] [PATCH] tools/libxl: Added vga
parameter for hvm domUs"):
From: Fabio Fantoni <fabio.fantoni@xxxxxxxxxx>
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@xxxxxxxxxx>
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))
