[libvirt] [PATCH] virDomainDefCopy: Skip ostype checks

[Ján Tomko]

On Mon, Jun 04, 2018 at 12:23:50PM +0200, Michal Privoznik wrote:
>On 06/02/2018 02:23 PM, Ján Tomko wrote:
>> On Sat, Jun 02, 2018 at 12:57:39PM +0200, Michal Privoznik wrote:
>>> When parsing domain XML the virCapsDomainData lookup is performed
>>> in order to fill in missing def->os.arch and def->os.machine
>>> strings. Well, when doing copy of already existing virDomainDef
>>> we don't want any automagic fill in of defaults (and those two
>>> strings are going to be provided at this point anyway by first
>>> parse of the domain XML).
>>
>> The sentence in parentheses documents an inefficiency -
>> If we have machine and arch, the DomainDataLookup call is pointless,
>> unless we want to validate it.
>>
>>>
>>> What is even worse is that we do not look up capabilities for
>>> parsed emulator path rather than some generic capabilities for
>>
>> s/than//
>>
>>> parsed arch. Therefore, if emulator points to qemu under
>>> non-default path (say $HOME/qemu-system-arm) but there's no such
>>> qemu under the default path (say /usr/bin/qemu-system-arm) the
>>> capabilities lookup fails and creating the copy is denied.
>>>
>>
>> This is downright a bug - we should be using the specified emulator
>> path to fill the machine type even for new domains. Are you able
>> to define a new domain without having a default emulator for it?
>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>> src/conf/domain_conf.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 86814d5f64..f36a1bfe79 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -28397,7 +28397,8 @@ virDomainDefCopy(virDomainDefPtr src,
>>>     virDomainDefPtr ret;
>>>     unsigned int format_flags = VIR_DOMAIN_DEF_FORMAT_SECURE;
>>>     unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
>>> -                               VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
>>> +                               VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
>>> +                               VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS;
>>
>> For missing QEMU emulator, we postpone all the stuff depending
>> on qemuCaps until domain startup:
>> commit 7726d1581f9e433a106f45ed87ec95ece575f817
>>    qemu: Implement postParse callback skipping on config reload
>>
>> Ideally, we'd remove this flag, move the automagic to PostParse
>> and make it non-fatal in the same way, but I don't know how much would
>> that break.
>
>Thing is, virCaps is going to be empty at this point. I mean, there is
>no default emulator. We can try to look up virQEMUCaps and probably
>derive something from that but that would only work for qemu driver.
>Anyway, I'll try to write a follow up patch that does something among
>those lines.

Oh, so even if we passed the custom emulator, we would not probe for it
if it's not present in the capabilities.

>
>>
>> Practically, this hunk is an improvement. Hoping you'll address the
>> newly-defined domain bug as well:
>
>You mean like this?
>

That is an improvement for those with both machine and arch specified.
Guess we can declare the rest a corner case and not worry about it.

>diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
>index f36a1bfe79..9f4ed22701 100644
>--- i/src/conf/domain_conf.c
>+++ w/src/conf/domain_conf.c
>@@ -19045,7 +19045,8 @@ virDomainDefParseXML(xmlDocPtr xml,
>     def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt);
>     def->emulator = virXPathString("string(./devices/emulator[1])", ctxt);
>
>-    if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
>+    if ((!def->os.arch || !def->os.machine) &&
>+        !(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
>         /* If the logic here seems fairly arbitrary, that's because it is :)
>          * This is duplicating how the code worked before
>          * CapabilitiesDomainDataLookup was added. We can simplify this,
>

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180604/81e9b1c8/attachment-0001.sig>