Re: [libvirt] [RFC PATCH 4/7] conf: Introduce new <hostdev> attribute '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.

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