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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Dec 16, 2013 at 12:17:02PM +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.
> 

No call site ever error-checks this. Probably a separate patch to add
error check to all of them?

> > +        if (libxl_defbool_val(vnc_enabled)) {
> > +            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.
> 

Ack.

> > +            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 ?
> 

I'm fine with either way. It's maintainers' call now. :-)

> > +            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.

Wei.

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

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