Re: [Xen-devel] [PATCH v10] tools/libxl: Add qxl vga interface support for upstream qemu

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

fantonifabio@xxxxxxxxxx writes ("[Xen-devel] [PATCH v10] tools/libxl: Add qxl 
vga interface support for upstream qemu"):
> Changes from v8:
> - vga=qxl instead of qxl=1 to use it.
> - Show an error and exit if vga="qxl" without qemu upstream.
> - Other small improvements.

This looks plausible to me.  I'd like an ack from Stefano.

Also this patch has some more of the formatting problems I mentioned
before and which Ian C has expanded on:

> +        if (b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_QXL) {
> +            if (b_info->device_model_version ==
> +               LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +                if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) {
> +                    b_info->video_memkb = (128 * 1024);
> +                }else if (b_info->video_memkb < (128 * 1024)) {

                    ^ extra space neeeded

Also it would be better for the 128MB figure to occur only once in the
source code, perhaps with a const or a #define.

> +                    LOG(ERROR,
> +                    "128 Mib videoram is the minimum for qxl default");

                           ^ should be indented to here; you can
                             use "a" "b" style string concatenation if
                             that's convenient to split the string
                             across multiple lines

> +        case LIBXL_VGA_INTERFACE_TYPE_QXL:
> +            /*QXL have 2 ram regions, ram and vram*/
                 ^ spaces here and                   ^ here
> +            flexarray_vappend(dm_args, "-vga", "qxl", NULL);
> +            if (b_info->video_memkb) {
> +                flexarray_vappend(dm_args, "-global",
> +                libxl__sprintf(gc, "qxl-vga.vram_size_mb=%lu",
> +                (b_info->video_memkb/2/1024)), "-global",
> +                libxl__sprintf(gc, "qxl-vga.ram_size_mb=%lu",
> +                (b_info->video_memkb/2/1024)), NULL);

This needs indentation to the right level, in each case to where the
relevant parens open.  Also you will probably find GCSPRINTF a helpful
macro which will make this a lot shorter.

> +            }else if (!strcmp(buf, "qxl")) {

                ^ space

> +                b_info->u.hvm.vga.kind
> +                = LIBXL_VGA_INTERFACE_TYPE_QXL;

                       ^ indentation needs to be to here

I appreciate that these will seem like nits, and that there are
existing places where the indentation and formatting is wrong, but
they are things that the reader's eye trips over and we are trying to
fix them, not introduce more of them.

Thanks,
Ian.

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