Re: [Xen-devel] [PATCH] xl: create VFB for PV guest when VNC is specified

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-16 at 16:42 +0000, Wei Liu wrote:
> This replicates a Xend behavior, when you specify:
> 
>   vnc=1
>   vnclisten=XXXX
>   vncpasswd=XXXX
> 
> in a PV guest's config file, it creates a VFB for you.
> 
> Fixes bug #25.
> http://bugs.xenproject.org/xen/bug/25
> 
> Reported-by: Konrad Wilk <konrad.wilk@xxxxxxxxxx>
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> ---
> Changes in V2:
> * use macros to reduce code duplication
> * vfb=[] take precedence over top level VNC options
> ---
> ---
>  tools/libxl/xl_cmdimpl.c |   96 
> ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 72 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index bd26bcc..f3f4d71 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1357,11 +1357,37 @@ skip_nic:
>          fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not 
> supported by xl\n");
>      }
>  
> +#define add_vfb_vkb(vfb,vkb)                                            \

I think what Ian was suggesting was a single macro which could expand an
arbitrary array given as a parameter, rather than this, which is pretty
fuggly. e.g.
        vfb = ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs)
or something (making liberal use of typeof() etc). Perhaps even
                vfb = ARRAY_EXTEND(d_config, vfbs)
and some cpp token pasting.

Once you have that you can use it for both vkb and vfb.

I think you would be better off skipping the ->devid set, but if you
want to keep it you'd want to call it something more suitable like
NEW_DEVICE() perhaps.

You don't need to use the new macro everywhere right now during code
freeze but we can flip everything in 4.5.

> @@ -1608,6 +1624,47 @@ skip_vfb:
>  
>  #undef parse_extra_args
>  
> +#define parse_top_level_vnc_options(enable,listen,passwd,display,unused) \

I think this could be a function, which is would be preferable to a
macro if possible.

Ian.


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