[libvirt] [RFC PATCH 4/7] conf: Introduce new <hostdev> attribute 'display'

[John Ferlan]

[...]

         if (mdevsrc->display)
>>
>> ->display > VIR_TRISTATE_SWITCH_ABSENT
>>
>>> +                virBufferAsprintf(buf, " display='%s'",
>>> +                                  virTristateSwitchTypeToString(mdevsrc->display));
>>> +        }
>>> +
>>>      }
>>>      virBufferAddLit(buf, ">\n");
>>>      virBufferAdjustIndent(buf, 2);
>>
>> What about a virDomainHostdevDefCheckABIStability check that display
>> type didn't change?
> 
> I'm sorry, I'm failing to see how it could change, the way I designed it is that
> whenever QEMU supports the capability we always default to 'off' which means
> that it'll always get formatted explicitly as 'off', even if the attribute was
> missing in the source XML.
> The only problem would an older libvirt who doesn't know what to do with that
> attribute, so it would ignore it which could cause issues with a newer QEMU
> without the patch linked below, but the ABI stability check wouldn't help
> anyway.
> 

OK - The ABI stuff is sometimes forgotten and it's just one of those
things I like to make sure about.  Seems like we're good without it.

>>
>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>> index 8493dfdd76..123a676ab9 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -382,6 +382,7 @@ typedef struct _virDomainHostdevSubsysMediatedDev virDomainHostdevSubsysMediated
>>>  typedef virDomainHostdevSubsysMediatedDev *virDomainHostdevSubsysMediatedDevPtr;
>>>  struct _virDomainHostdevSubsysMediatedDev {
>>>      int model;                          /* enum virMediatedDeviceModelType */
>>> +    int display; /* virTristateSwitchType */
>>>      char uuidstr[VIR_UUID_STRING_BUFLEN];   /* mediated device's uuid string */
>>>  };
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 2c51e4c0d8..27088d4456 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -4277,10 +4277,35 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
>>>  }
>>>
>>>
>>> +static int
>>> +qemuDomainDeviceDefValidateHostdevMdev(const virDomainHostdevSubsysMediatedDev *mdevsrc,
>>> +                                       const virDomainDef *def)
>>> +{
>>> +    if (mdevsrc->display) {
>>
>>> VIR_TRISTATE_SWITCH_ABSENT
>>
>>> +        if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("<hostdev> attribute 'display' is only supported"
>>> +                             " with model='vfio-pci'"));
>>> +
>>> +            return -1;
>>> +        } else if (def->ngraphics == 0) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("graphics device is needed for attribute value "
>>> +                             "'display=on' in <hostdev>"));
>>
>> Hmmm.. not sure we noted that in the formatdomain - probably should.
>>
>> Also if this were a PostParse check, then would the xml2xml tests fail
>> if no graphics were defined (e.g. hostdev-mdev-display-missing-graphics.xml)
> 
> Right, good point, I preferred validation instead of post parse so as not to
> risk 'loosing' a domain, but it doesn't make any sense wanting a display and
> not having any graphical framebuffer, I'll change that.
> 

When you're adding new options, going with PostParse I think is fine.
The Validate pass was added to catch those cases where we found
something after introduction of an option but forgot to check validity
so we have to check "later" so we don't have a domain go missing.

John

[...]