On 7/3/24 18:29, John Levon wrote:
On Wed, Jul 03, 2024 at 09:44:47AM +0200, Michal Prívozník wrote:
LIVE is not supported and thus shouldn't be in list of supported flags.
I was a bit unclear on the test hypervisor case - in a sense everything is both LIVE *and* CONFIG :)
I see what you mean, but no. The fact that libvirt doesn't really store XMLs on disk for inactive domains doesn't necessarily mean there's no distinction. The aim of test driver is to mimic real life scenarios as closely as possible, so that when somebody is developing an app on top of libvirt they can use the test driver instead of spawning real VMs and thus test their app quickly. Therefore, the distinction between inactive and live XMLs must be preserved.
I get the rationale, but now we're inconsistent with our previous contribution:
10247 static int 10248 testDomainAttachDeviceLiveAndConfig(virDomainObj *vm, 10249 testDriver *driver, 10250 const char *xml, 10251 unsigned int flags) 10252 { 10253 g_autoptr(virDomainDeviceDef) devLive = NULL; 10254 unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | 10255 VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; 10256 10257 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
Given that the test driver does not and cannot have a persisted configuration (there is only the in-process ->vmdef), I'm confused why you made the new code be:
10399 virCheckFlags(VIR_DOMAIN_AFFECT_CONFIG, -1);
in testDomainUpdateDeviceFlags().
The fact that domain XML is not stored on a disk doesn't necessarily mean it can't have inactive XML. Those are two different things. Just consider: virsh -c test:///default Welcome to virsh, the virtualization interactive terminal. Type: 'help' for help with commands 'quit' to quit virsh # domstate test running virsh # destroy test Domain 'test' destroyed virsh # domstate test shut off And in fact, inactive (sometimes called also config) XML can differ from that of a running domain. Hence '--inactive' flag to 'dumpxml'. And since the testDomainUpdateDeviceFlags() is made to error out on VIR_DOMAIN_AFFECT_LIVE flag it makes no sense to accept the flag. Therefore it's not listed in virCheckFlags(). There's one caveat - if the API is called with flags=0 then it means "affect current state of the domain". flgas is then updated in virDomainObjUpdateModificationImpact() and thus _LIVE needs to be checked again.
For our application we actually do want to pass the LIVE flag - we never want to modify any persistent configuration. We'd prefer not to have to carry a patch on top for this.
Ah, I thought you're interested only in _CONFIG since that's what your original patch implemented. But implementing live update should be pretty trivial. It's a test driver so "hypervisor" will allow just any change. Do you want to post a patch for that or should I? Michal