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
Attachment:
pgpDfzqJ3msfM.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list