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:36:31PM +0000, Ian Campbell wrote:
> On Mon, 2013-12-16 at 12:17 +0000, Ian Jackson wrote:
[...]
> 
> > > +            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.
> 

Yes this is done on purpose as I don't know the complete set of possible
user config files. Probably Konrad can let us know what he expects.

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

I did look at Xend code, if there's top VNC option present it creates a
VFB. That's why I said "replicates" a Xend behavior in commit message.
I didn't go further than that though so I don't know one take precedence
over the other.

> > > +            vfb = d_config->vfbs + d_config->num_vfbs;
> > > +            libxl_device_vfb_init(vfb);
> > > +            vfb->devid = d_config->num_vfbs;
> > > +
[...]
> > > +            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 .
> 

with the what?

Wei.

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

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