Re: [PATCH] test_driver: add testUpdateDeviceFlags implementation
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