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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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

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

> +
> +            d_config->num_vfbs++;
> +            d_config->num_vkbs++;
> +        }
> +    }

Thanks,
Ian.


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