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 12:17 +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] xl: create VFB for PV guest when VNC is specified"):
> > This replicate a Xend behavior, when you specify:
> 
> Thanks.  I think the principle is sound.  I have some quibbles.
> 
> > +    if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
> > +        libxl_defbool vnc_enabled;
> > +
> > +        xlu_cfg_get_defbool(config, "vnc", &vnc_enabled, 0);
> 
> Missing error check.
> 
> > +        if (libxl_defbool_val(vnc_enabled)) {

The result of xlu_cfg_get_defbool can be neither true nor false, it can
be "the default", which will cause this function to fail (abort even?).

I think you want xlu_cfg_get_long here and to use the normal boolean
logic.



> > +            libxl_device_vfb *vfb;
> > +            libxl_device_vkb *vkb;
> > +
> > +            d_config->vfbs = (libxl_device_vfb *)
> > +                realloc(d_config->vfbs, sizeof(libxl_device_vfb) * 
> > (d_config->num_vfbs + 1));
> 
> This should be xrealloc, and wrapped to 80 columns.  But see below.

This also suggests that we might be trying to support both the
"toplevel" vnc options and the "vfb = []" style at the same time.

IMHO the vfb option should take precedence -- i.e. we should ignore the
toplevel options if it is present.

The alternative would be some sort of union of the toplevel options and
the first vfb given, but that sounds a bit mad...

I suppose there is also the question of what xend did here.

> > +            vfb = d_config->vfbs + d_config->num_vfbs;
> > +            libxl_device_vfb_init(vfb);
> > +            vfb->devid = d_config->num_vfbs;
> > +
> > +            d_config->vkbs = (libxl_device_vkb *)
> > +                realloc(d_config->vkbs, sizeof(libxl_device_vkb) * 
> > (d_config->num_vkbs + 1));
> > +            vkb = d_config->vkbs + d_config->num_vkbs;
> > +            libxl_device_vkb_init(vkb);
> > +            vkb->devid = d_config->num_vkbs;
> > +
> > +            libxl_defbool_set(&vfb->vnc.enable, true);
> 
> There's a lot of this kind of boilerplate.  I'm tempted to suggest
> making a macro to do this.  Searching for "devid =" found 4 call sites
> if that line is included in the macro; searching for "realloc" found
> me 6 call sites if it isn't.  And that's before your two additional
> ones.  Ian C, what do you think ?

Some helpers for dealing with allocating and resizing/appending to libxl
Array types might be a useful addition to the library itself, i.e. idl
generated? Otherwise an xl macro might indeed be handy.

I'm not sure what the 4 vs. 6 "if that line is/isn't included in" is
referring too though.

> > +            xlu_cfg_replace_string(config, "vnclisten", &vfb->vnc.listen, 
> > 0);
> > +            xlu_cfg_replace_string(config, "vncpasswd", &vfb->vnc.passwd, 
> > 0);
> > +            if (!xlu_cfg_get_long (config, "vncdisplay", &l, 0))
> > +                vfb->vnc.display = l;
> > +            xlu_cfg_get_defbool(config, "vncunused", &vfb->vnc.findunused, 
> > 0);
> 
> This duplicates the HVM VNC option parsing.  It should be factored out
> into a subroutine.

Ack.

That makes me wonder if the top level options shouldn't be populating
&b_info->u.pv.vnc.* (and if that shouldn't have been a non-keyed field)
while vfb = fills in d_config->vfbs. That is a bigger change on the
libxl side though and maybe doesn't make quite as much sense as with
the .

> 
> > +
> > +            d_config->num_vfbs++;
> > +            d_config->num_vkbs++;
> > +        }
> > +    }
> 
> Thanks,
> Ian.
> 



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