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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-16 at 17:39 +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 V3:
> * ARRAY_EXTEND macro
> * make parse_top_level_vnc_options a function
> 
> Changes in V2:
> * use macros to reduce code duplication
> * vfb=[] take precedence over top level VNC options
> ---
>  tools/libxl/xl_cmdimpl.c |   89 
> ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index bd26bcc..6a22e17 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -315,6 +315,13 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> +#define ARRAY_EXTEND(array,count)                                       \
> +    do {                                                                \
> +        (array) = xrealloc((array),                                     \
> +                           sizeof(typeof(*(array))) * ((count) + 1));   \

sizeof(typeof(*array)) == sizeof(*array), doesn't it?

> +        (count) = (count) + 1;                                          \

(count++)

I suppose in a macro like this it is tricky to arrange to only evaluate
the arguments once, since you need to read and write, but I think you
can arrange to evaluate array exactly once instead of 3 times and count
just once.

Perhaps Ian J has some hints.

> @@ -1369,13 +1393,13 @@ skip_nic:
>              char *buf2 = strdup(buf);
>              char *p, *p2;
>  
> -            d_config->vfbs = (libxl_device_vfb *) realloc(d_config->vfbs, 
> sizeof(libxl_device_vfb) * (d_config->num_vfbs + 1));
> -            vfb = d_config->vfbs + d_config->num_vfbs;
> +            ARRAY_EXTEND(d_config->vfbs, d_config->num_vfbs);
> +            vfb = d_config->vfbs + (d_config->num_vfbs - 1);

You should make the macro return the new element like I suggested. Apart
from being convenient it avoids having to have this logic, and the
potential for an off-by-one error, repeated in every user.

> @@ -1608,6 +1630,44 @@ skip_vfb:
>  
>  #undef parse_extra_args
>  
> +    /* If we've already got vfb=[] for PV guestthen ignore top level

"guest then"

> +            parse_top_level_vnc_options(config,
> +                                        &vfb->vnc.enable,

This helper can take a libxl_vnc * instead of all of these, individual
fields. Pass it &vfb->vnc or &b_Info->u.hvm.vnc as appropriate.


Ian.


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