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
