[libvirt] [libvirt-designer PATCHv3 6/9] Check for vioserial availability before adding a SPICE agent

[Christophe Fergeau]

On Tue, Jun 04, 2013 at 07:10:57PM +0200, Michal Privoznik wrote:
> On 04.06.2013 16:33, Christophe Fergeau wrote:
> > This makes use of the new gvir_designer_domain_get_supported_devices()
> > method.
> > ---
> >  libvirt-designer/libvirt-designer-domain.c | 42 +++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libvirt-designer/libvirt-designer-domain.c b/libvirt-designer/libvirt-designer-domain.c
> > index 1d37e21..7466ee9 100644
> > --- a/libvirt-designer/libvirt-designer-domain.c
> > +++ b/libvirt-designer/libvirt-designer-domain.c
> > @@ -70,6 +70,7 @@ static gboolean error_is_set(GError **error)
> >  }
> >  
> >  static const char GVIR_DESIGNER_SPICE_CHANNEL_NAME[] = "com.redhat.spice.0";
> > +static const char GVIR_DESIGNER_SPICE_CHANNEL_DEVICE_ID[] = "http://pciids.sourceforge.net/v2.2/pci.ids/1af4/1003";
> >  static const char GVIR_DESIGNER_VIRTIO_BLOCK_DEVICE_ID[] = "http://pciids.sourceforge.net/v2.2/pci.ids/1af4/1001";
> >  
> >  enum {
> > @@ -410,12 +411,45 @@ gvir_designer_domain_has_spice_channel(GVirDesignerDomain *design)
> >  }
> >  
> >  
> > -static void gvir_designer_domain_add_spice_channel(GVirDesignerDomain *design)
> > +static gboolean
> > +gvir_designer_domain_supports_spice_channel(GVirDesignerDomain *design)
> > +{
> > +    OsinfoDeviceList *devices;
> > +    OsinfoFilter *filter;
> > +    gboolean vioserial_found = FALSE;
> > +
> > +    filter = osinfo_filter_new();
> > +    osinfo_filter_add_constraint(filter,
> > +                                 OSINFO_ENTITY_PROP_ID,
> > +                                 GVIR_DESIGNER_SPICE_CHANNEL_DEVICE_ID);
> > +    devices = gvir_designer_domain_get_supported_devices(design, filter);
> > +    if (devices) {
> > +        g_warn_if_fail(osinfo_list_get_length(OSINFO_LIST(devices)) <= 1);
> 
> so warn if the list length is greater than 1 ...
> 
> > +        if (osinfo_list_get_length(OSINFO_LIST(devices)) >= 1)
> > +            vioserial_found = TRUE;
> 
> ... but that's the only way to set vioserial_found to TRUE. Or am I
> missing something?

Hmm, I think I wanted to get a warning if the list has 2 items or more, as
this is very unexpected, but still do something sensible when that happens.
In other words, if the list length is 0, then vioserial_found is FALSE, if
it's 1, it's TRUE, if it's more than 1, then we warn, but still set it to
TRUE. I agree the g_warn_if_fail() should be removed, or that there should
be a comment with an explanation.



> >  
> > @@ -516,7 +552,7 @@ gvir_designer_domain_add_graphics(GVirDesignerDomain *design,
> >                                                                  GVIR_CONFIG_DOMAIN_GRAPHICS_SPICE_IMAGE_COMPRESSION_OFF);
> >          graphics = GVIR_CONFIG_DOMAIN_GRAPHICS(spice);
> >          if (!gvir_designer_domain_has_spice_channel(design))
> > -            gvir_designer_domain_add_spice_channel(design);
> > +            gvir_designer_domain_add_spice_channel(design, NULL);
> 
> I think we want s/NULL/error/ here.

Not sure about that, if we do that, we would have to return NULL from
_add_graphics when adding the channel fails. Some people are using SPICE on
xen if I'm not mistaken, adding a spice channel will fail there. Probably a
bit far fetched at this point, so we could fail for now until someone comes
to us with such a use case ;)

Thanks for the review,

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130605/dba72e00/attachment-0001.sig>