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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: 24 October 2013 2:14 PM
> To: Rob Hoes
> Cc: xen-devel@xxxxxxxxxxxxx; Ian Jackson; Stefano Stabellini; Anthony Perard
> Subject: Re: [RESEND] libxl: Allow 4 MB of video RAM for Cirrus graphics on
> traditional QEMU
> 
> 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.

Thanks Ian. I'll fix up those style issues.

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

I used "will" here because it is the QEMU later on doing the ignoring, not this 
particular bit of code. How about making it active this like: "Traditional QEMU 
will ignore videoram other than 4 MB for CIRRUS"?

Cheers,
Rob

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