[libvirt] [PATCH] virDomainDefCopy: Skip ostype checks

Michal Privoznik posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/863137d224847f9f2be989fec32a3b8d11f44db2.1527937059.git.mprivozn@redhat.com
Test syntax-check passed
src/conf/domain_conf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[libvirt] [PATCH] virDomainDefCopy: Skip ostype checks
Posted by Michal Privoznik 5 years, 9 months ago
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).

What is even worse is that we do not look up capabilities for
parsed emulator path rather than some generic capabilities for
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.

Signed-off-by: Michal Privoznik <mprivozn@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;
 
     if (migratable)
         format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE | VIR_DOMAIN_DEF_FORMAT_MIGRATABLE;
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virDomainDefCopy: Skip ostype checks
Posted by Ján Tomko 5 years, 9 months ago
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@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.

Practically, this hunk is an improvement. Hoping you'll address the
newly-defined domain bug as well:
Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virDomainDefCopy: Skip ostype checks
Posted by Michal Privoznik 5 years, 9 months ago
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@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.

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

You mean like this?

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,


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virDomainDefCopy: Skip ostype checks
Posted by Ján Tomko 5 years, 9 months ago
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@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@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list