Re: [Xen-devel] [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on traditional QEMU

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2013-10-21 at 13:41 +0100, Rob Hoes wrote:
> VMs using Cirrus graphics have always had 4 MB of video RAM on XenServer, 
> while
> libxl currently does not allow values less than 8 MB.
> 
> Signed-off-by: Rob Hoes <rob.hoes@xxxxxxxxxx>
> 
> ----
> Resend note: No one has replied with an Acked-by line yet,

I'd like to see an ack from one of the qemu guys regarding this change.

From my PoV the logic looks correct but there are some coding stylei
issues which I'll comment on below.

> +        switch (b_info->device_model_version) {
> +        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +            switch (b_info->u.hvm.vga.kind) {
> +            case LIBXL_VGA_INTERFACE_TYPE_STD:
> +                if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> +                    b_info->video_memkb = 8 * 1024;
> +                if (b_info->video_memkb < (8 * 1024) ){
> +                    LOG(ERROR,"videoram must be at least 8 MB for STDVGA "

Coding style would have a space after the "," (multiple cases of this).
Also the brackets around 8*1024 are not necessary, and the space at
theend should be after the closing ) (before the {) i.e.
+                if (b_info->video_memkb < 8 * 1024) {
+                    LOG(ERROR,"videoram must be at least 8 MB for STDVGA  
...");

I prefer to avoid splitting string constants even if this violates the
80 column limit, this makes it easier to grep.

> +                        "on QEMU_XEN_TRADITIONAL");
> +                    return ERROR_INVAL;
> +                }
> +                break;
> +            case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
> +            default:
> +                if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> +                    b_info->video_memkb = 4 * 1024;
> +                if (b_info->video_memkb != (4 * 1024) ){
> +                    LOG(WARN,"videoram other than 4 MB for CIRRUS on "
> +                        "QEMU_XEN_TRADITIONAL will be ignored"); }

Closing brace should be on a new line. Actually, since this is a single
statement the braces are not needed.

Rather than "will be" I might word this in a more active voice like
"Ignoring videoram blah blah".

Ian.


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