Re: [libvirt] [PATCH 1/2] libxl: add acpi slic table support

[Jim Fehlig]

On 9/10/19 5:24 PM, Marek Marczykowski-Górecki  wrote:
> On Tue, Sep 10, 2019 at 10:54:15PM +, Jim Fehlig wrote:
>> On 9/6/19 8:31 PM, Marek Marczykowski-Górecki  wrote:
>>> From: Ivan Kardykov 
>>>
>>> Libxl driver did not support setup additional acpi firmware to xen
>>> guest. It is necessary to activate OEM Windows installs. This patch
>>> allow to define in OS section acpi table param (which supported domain
>>> common schema).
>>>
>>> Signed-off-by: Ivan Kardykov 
>>> [added info to docs/formatdomain.html.in]
>>> Signed-off-by: Marek Marczykowski-Górecki 
>>> ---
>>>docs/formatdomain.html.in | 3 ++-
>>>src/libxl/libxl_conf.c| 5 +
>>>2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index fcb7c59c00..de612ae870 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -363,7 +363,8 @@
>>>  The table element contains a fully-qualified path
>>>to the ACPI table. The type attribute contains the
>>>ACPI table type (currently only slic is supported)
>>> -Since 1.3.5 (QEMU only)
>>> +Since 1.3.5 (QEM)
>>
>> You removed one too many characters :-). s/QEM/QEMU/
>>
>>> +Since 5.8.0 (Xen)
>>>
>>>
>>>Container boot
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 766a726ebc..c1e248d98c 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -506,6 +506,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>>>  def->features[VIR_DOMAIN_FEATURE_ACPI] ==
>>>  VIR_TRISTATE_SWITCH_ON);
>>>
>>> +/* copy SLIC table path to acpi_firmware */
>>> +if (def->os.slic_table &&
>>> +VIR_STRDUP(b_info->u.hvm.acpi_firmware, 
>>> def->os.slic_table) < 0)
>>> +return -1;
>>> +
>>
>> Is 'acpi_firmware=' the xl.cfg equivalent setting? If so we'll want it added 
>> to
>> the domXML<->xl.cfg converter (which now lives in the src/libxl/ directory).
> 
> Functionally yes. But acpi_firmware= is about generic ACPI table, not
> only SLIC. This means xl.cfg acpi_firmware= converted to domXML may be
> misleading. Is it a problem?

I don't think it's a problem. But let me ask another way: How would you specify 
the SLIC in xl.cfg? I.e., what would a comparable xl.cfg snippet look like?

Regards,
Jim

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

Re: [libvirt] [PATCH 1/2] libxl: add acpi slic table support

[Marek Marczykowski-Górecki]

On Tue, Sep 10, 2019 at 10:54:15PM +, Jim Fehlig wrote:
> On 9/6/19 8:31 PM, Marek Marczykowski-Górecki  wrote:
> > From: Ivan Kardykov 
> > 
> > Libxl driver did not support setup additional acpi firmware to xen
> > guest. It is necessary to activate OEM Windows installs. This patch
> > allow to define in OS section acpi table param (which supported domain
> > common schema).
> > 
> > Signed-off-by: Ivan Kardykov 
> > [added info to docs/formatdomain.html.in]
> > Signed-off-by: Marek Marczykowski-Górecki 
> > ---
> >   docs/formatdomain.html.in | 3 ++-
> >   src/libxl/libxl_conf.c| 5 +
> >   2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index fcb7c59c00..de612ae870 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -363,7 +363,8 @@
> > The table element contains a fully-qualified path
> >   to the ACPI table. The type attribute contains the
> >   ACPI table type (currently only slic is supported)
> > -Since 1.3.5 (QEMU only)
> > +Since 1.3.5 (QEM)
> 
> You removed one too many characters :-). s/QEM/QEMU/
> 
> > +Since 5.8.0 (Xen)
> >   
> >   
> >   Container boot
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index 766a726ebc..c1e248d98c 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -506,6 +506,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> > def->features[VIR_DOMAIN_FEATURE_ACPI] ==
> > VIR_TRISTATE_SWITCH_ON);
> >   
> > +/* copy SLIC table path to acpi_firmware */
> > +if (def->os.slic_table &&
> > +VIR_STRDUP(b_info->u.hvm.acpi_firmware, 
> > def->os.slic_table) < 0)
> > +return -1;
> > +
> 
> Is 'acpi_firmware=' the xl.cfg equivalent setting? If so we'll want it added 
> to 
> the domXML<->xl.cfg converter (which now lives in the src/libxl/ directory).

Functionally yes. But acpi_firmware= is about generic ACPI table, not
only SLIC. This means xl.cfg acpi_firmware= converted to domXML may be
misleading. Is it a problem?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] libxl: add acpi slic table support

[Jim Fehlig]

On 9/6/19 8:31 PM, Marek Marczykowski-Górecki  wrote:
> From: Ivan Kardykov 
> 
> Libxl driver did not support setup additional acpi firmware to xen
> guest. It is necessary to activate OEM Windows installs. This patch
> allow to define in OS section acpi table param (which supported domain
> common schema).
> 
> Signed-off-by: Ivan Kardykov 
> [added info to docs/formatdomain.html.in]
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
>   docs/formatdomain.html.in | 3 ++-
>   src/libxl/libxl_conf.c| 5 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fcb7c59c00..de612ae870 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -363,7 +363,8 @@
> The table element contains a fully-qualified path
>   to the ACPI table. The type attribute contains the
>   ACPI table type (currently only slic is supported)
> -Since 1.3.5 (QEMU only)
> +Since 1.3.5 (QEM)

You removed one too many characters :-). s/QEM/QEMU/

> +Since 5.8.0 (Xen)
>   
>   
>   Container boot
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 766a726ebc..c1e248d98c 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -506,6 +506,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> def->features[VIR_DOMAIN_FEATURE_ACPI] ==
> VIR_TRISTATE_SWITCH_ON);
>   
> +/* copy SLIC table path to acpi_firmware */
> +if (def->os.slic_table &&
> +VIR_STRDUP(b_info->u.hvm.acpi_firmware, def->os.slic_table) 
> < 0)
> +return -1;
> +

Is 'acpi_firmware=' the xl.cfg equivalent setting? If so we'll want it added to 
the domXML<->xl.cfg converter (which now lives in the src/libxl/ directory).

Regards,
Jim

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

Re: [libvirt] [Libvirt-ci] Build failed in Jenkins: libosinfo-build » libvirt-fedora-rawhide #231

[Fabiano Fidêncio]

[snip]

> *** error: gettext infrastructure mismatch: using a Makefile.in.in from 
> gettext version 0.19 but the autoconf macros are from gettext version 0.20
> gmake[2]: *** [Makefile:237: stamp-po] Error 1
> gmake[2]: Leaving directory 
> '
> gmake[1]: *** [Makefile:587: all-recursive] Error 1
> gmake[1]: Leaving directory 
> '
> gmake: *** [Makefile:473: all] Error 2
> Build step 'Execute shell' marked build as failure
>

JFTR: Instead of spending time trying to figure out what's going on
here, let's just get meson patches merged to libosinfo as, with that,
we'll get rid of this for free.

Ci: https://gitlab.com/fidencio/libosinfo/pipelines/81396029

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

Re: [libvirt] [PATCH v4 0/8] CPU Model Baseline and Comparison for s390x

[Collin Walling]

On 9/10/19 11:38 AM, David Hildenbrand wrote:

On 10.09.19 17:22, Jiri Denemark wrote:

On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:

On 8/20/19 10:06 AM, Jiri Denemark wrote:

First, let me apologize for such a late review. I'll try my best to
review your series earlier next time.



Your review is greatly appreciated! I haven't replied to your other
posts on this series as my comments were mostly acknowledgements rather
than discussion pieces. I'm working through them.


On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:

When baselining CPU models and the user appends the --features argument
to the command, s390x will only report back features that supersede the
base model definition.

*NOTE* if the --features flag is intended to expand /ALL/ features
available to a CPU model (such as the huge list of features reported
by a full CPU model expansion), please let me know and I can resolve
this.


I'm not sure how well this fits s390 world, but the semantics of
VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
features which are hidden behind the CPU model. That is, all feature
which you'd get when starting QEMU with just the CPU model name and no
additional features. The extra features should not be touched at all.
Specifically, removing them when the flag is not used is wrong.

To me this looks like the flag should really result in running
query-cpu-model-expansion (but likely the "static" one rather then
"full" expansion) on the baseline CPU model and reporting the enabled
features along with those already included in the baseline feature set.



Actually, query-cpu-model-baseline will return a CPU model along with a
feature set. The features returned are the same as those produced from a
static expansion on the model.

Correct me if I am wrong here: virsh should report features *only* if the
--features flag is present. Otherwise, we only report the model name (which
can be accomplished by stripping the result of all reported features).


No, this is wrong in general (although it maybe correct for s390, I
don't know).


David here: Not correct for s390x



Let me explain with some hypothetical CPU models.

Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
Model2 will enable f1, f2, f3, f4, f5.
Model3 will enable f1, f2, f3, f4, f6.

Running baseline command for [Model1, Model2, Model3] should return
Model1 with no additional features as [f1, f2, f3] are in only ones
common to all three models and they are all enabled by Model1.

However, baseline of [Model2, Model3] should return Model1 + f4. The
common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
f2, f3] only, which means we need to add f4 explicitly.

With --features, even the features enabled implicitly by a specific CPU
model should be returned. That is, the first result would be Model1 +
[f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].

In other words, stripping all features if no --features option is used
is wrong. Only features implicitly enabled by the CPU model should be
stripped.

Specifically, baseline without --features on the following CPU
definition

 
   Model1
   
 

should be the same definition (i.e., Model1 + f4).


s390x will fallback to base models when baselining/expanding ("minimum
feature set we expect to be around for a certain cpu generation", which
is independent of the QEMU version and will never change). But if you're
already using base models, it works as you would expect it.

E.g.

Model1 is [f1, f2, f3]
Model1-base is [f1, f2]

Model2 is [f1, f2, f3, f4]
Model2-base is [f1, f2, f3]

Baselining Model1 with Model2 would result in
Model1-base + f3

For this, QMP "query-cpu-model-baseline" can be used.

Note: When expending cpu models (e.g. host), one would already always
get base models. So there, it would work completely as you expect it.

Now, to achieve the "--features" part, simply throw the result of
"query-cpu-model-baseline" into "query-cpu-model-expansion" with
"CpuModelExpansionType" == "full"



The question is whether you can enabled extra features on s390 or you
can only use plain CPU models with no additional features. If additional
features are allowed, I'd guess baseline without features should return
the result of the QMP command minus the features a static expansion of
the CPU model itself would return (this is assuming f4 will be included
in the result we get from QEMU for Model1 + f4). When --features is
used, the result from QEMU should be returned.


We do have plenty of extra cpu features on s390x. *Plenty* :)



Indeed, there's quite a few.



I hope it's clear now. However, I'm not sure how CPU models work on
s390 and I'd be happy if you could explain it to me. Especially, whether
something similar to what I described can happen there.

Jirka






Yes, both you and David have provided an excellent explanation of what's
expected (from both perspectives). Thank you both. I'll make 

Re: [libvirt] [PATCH] qemu: Fix regression in snapshot-revert

[Daniel Henrique Barboza]

On 9/10/19 3:59 PM, Eric Blake wrote:

On 9/9/19 5:17 PM, Daniel Henrique Barboza wrote:


On 9/9/19 5:52 PM, Eric Blake wrote:

Commit f10562799 introduced a regression: if reverting to a snapshot
fails early (such as when we refuse to revert to an external
snapshot), we lose track of the domain's current snapshot.

See: https://bugzilla.redhat.com/1738747
Signed-off-by: Eric Blake 
---

I don't understand why f10562799 broke this code like this - tried
looking the changes made in the commit and the "if (snap)" was
there since a long time, no changes were made in the 'snap'
variable as well - but this change didn't break anything else, so:

Reviewed-by: Daniel Henrique Barboza 

I'll update the commit message to give more details:

Before that patch, we maintained the notion of the current snapshot in
two places: vm->current_snapshot, and snap->def->current.  If you have a
domain with two snapshots, s1 and s2 (where s2 is current) and want to
revert to s1, the old code cleared the def->current flag on s2 before
noticing that reverting to s1 was not possible, but at least left
vm->current_snapshot unchanged.  That meant we had a _different_ bug -
the code was inconsistent on whether the domain had a current snapshot
(as long as libvirtd didn't shut down, vm->current_snapshot ruled, so s2
was still claimed as current; but if you restart libvirtd, the XML for
s2 didn't track that it was current, so after restart you'd have no
current snapshot).  After that patch, the code was unconditionally
changing vm->current_snapshot to NULL (but at least was consistent
everywhere else that the XML never got out of sync with the notion of
which snapshot was current).  It also didn't help that after that patch,
the code clearing the snapshot occurs twice in the function - once right
after determining that the early checks have succeeded, the other
unconditionally on all failure paths.

So the fix is as simple as removing the unconditional clearing of s2 as
the current snapshot in the cleanup code, in favor of the earlier
clearing that happens only after the early checks succeed.


Thanks for the explanation!





ps: I think adding a test case for this failure (in tests/virsh-snapshot ?)
is worth considering, especially considering that we changes from
from Maxiwell (adding an inactive XML to the snapshot) that can
add up more complexity in the snapshot mechanics.

I tried. But the test driver doesn't forbid 'snapshot-revert' on
external snapshots, and also doesn't try to write state to XML files, so
it never copied the problematic code from the qemu driver that could
even trigger the bug.


I see. But now that I understood what you did and how the code is
neater, I changed my mind - I don't think Maxiwell's changes are going to
be too hard of a hit there. We can enhance the test_driver to
behave like the regular driver in this case of external snapshots, and
add a test case for this scenario, in another day.


Thanks,


DHB





   src/qemu/qemu_driver.c | 2 --
   1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b28a26c3d6..093b15f500 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16941,8 +16941,6 @@
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
   virDomainSnapshotSetCurrent(vm->snapshots, NULL);
   ret = -1;
   }
-    } else if (snap) {
-    virDomainSnapshotSetCurrent(vm->snapshots, NULL);
   }
   if (ret == 0 && config && vm->persistent &&
   !(ret = virDomainSaveConfig(cfg->configDir, driver->caps,




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

Re: [libvirt] [PATCH 0/7] qemu_command: clean up more labels (virtio-fs epopee)

[Cole Robinson]

On 8/26/19 4:44 PM, Ján Tomko wrote:
> Ján Tomko (7):
>   qemuBuildHostNetStr: remove unused cfg
>   qemuBuildHostNetStr: remove unused 'driver' argument
>   qemuBuildHostNetStr: remove unnecessary cleanup label
>   qemuBuildSoundCommandLine: reduce scope of codecstr
>   qemuBuildMemoryBackendProps: remove useless cleanup label
>   qemuBuildMemoryBackendProps: use 'rc' instead of ret.
>   qemuBuildMemoryCellBackendStr: remove useless ret variable
> 
>  src/qemu/qemu_command.c | 87 ++---
>  src/qemu/qemu_command.h |  1 -
>  src/qemu/qemu_hotplug.c |  2 +-
>  3 files changed, 38 insertions(+), 52 deletions(-)
> 

Patch #3 has some rebase conflicts after the slirp work, but they are
straight forward

Reviewed-by: Cole Robinson 

- Cole

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

Re: [libvirt] [PATCH] qemu: Fix regression in snapshot-revert

[Eric Blake]

On 9/9/19 5:17 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 9/9/19 5:52 PM, Eric Blake wrote:
>> Commit f10562799 introduced a regression: if reverting to a snapshot
>> fails early (such as when we refuse to revert to an external
>> snapshot), we lose track of the domain's current snapshot.
>>
>> See: https://bugzilla.redhat.com/1738747
>> Signed-off-by: Eric Blake 
>> ---
> 
> I don't understand why f10562799 broke this code like this - tried
> looking the changes made in the commit and the "if (snap)" was
> there since a long time, no changes were made in the 'snap'
> variable as well - but this change didn't break anything else, so:
> 
> Reviewed-by: Daniel Henrique Barboza 

I'll update the commit message to give more details:

Before that patch, we maintained the notion of the current snapshot in
two places: vm->current_snapshot, and snap->def->current.  If you have a
domain with two snapshots, s1 and s2 (where s2 is current) and want to
revert to s1, the old code cleared the def->current flag on s2 before
noticing that reverting to s1 was not possible, but at least left
vm->current_snapshot unchanged.  That meant we had a _different_ bug -
the code was inconsistent on whether the domain had a current snapshot
(as long as libvirtd didn't shut down, vm->current_snapshot ruled, so s2
was still claimed as current; but if you restart libvirtd, the XML for
s2 didn't track that it was current, so after restart you'd have no
current snapshot).  After that patch, the code was unconditionally
changing vm->current_snapshot to NULL (but at least was consistent
everywhere else that the XML never got out of sync with the notion of
which snapshot was current).  It also didn't help that after that patch,
the code clearing the snapshot occurs twice in the function - once right
after determining that the early checks have succeeded, the other
unconditionally on all failure paths.

So the fix is as simple as removing the unconditional clearing of s2 as
the current snapshot in the cleanup code, in favor of the earlier
clearing that happens only after the early checks succeed.

> 
> 
> ps: I think adding a test case for this failure (in tests/virsh-snapshot ?)
> is worth considering, especially considering that we changes from
> from Maxiwell (adding an inactive XML to the snapshot) that can
> add up more complexity in the snapshot mechanics.

I tried. But the test driver doesn't forbid 'snapshot-revert' on
external snapshots, and also doesn't try to write state to XML files, so
it never copied the problematic code from the qemu driver that could
even trigger the bug.


> 
>>   src/qemu/qemu_driver.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index b28a26c3d6..093b15f500 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -16941,8 +16941,6 @@
>> qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>   virDomainSnapshotSetCurrent(vm->snapshots, NULL);
>>   ret = -1;
>>   }
>> -    } else if (snap) {
>> -    virDomainSnapshotSetCurrent(vm->snapshots, NULL);
>>   }
>>   if (ret == 0 && config && vm->persistent &&
>>   !(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libvirt.spec.in: Add the Secure Boot-variant OVMF binaries

[Cole Robinson]

On 7/30/19 12:11 PM, Kashyap Chamarthy wrote:
> Currently the RPM spec doesn't add the 'secboot'-variant OVMF binaries
> (an unintentional omission, checking with Cole on #virt, OFTC) for
> 'x86_64' and 'ia32'.  Add them.
> 
> This way, getDomainCapabilities() will report all the OVMF binaries that
> are present on the system.  E.g. on Fedora 29, if you only have the
> edk2-ovmf-20190308stable-1.fc29.noarch package installed, then running
> `virsh domcapabilities` will enumerate _both_ the OVMF binaries (instead
> of just the OVMF_CODE.fd):
> 
>   $> virsh getdomcapabilities
> ...
> 
>   /usr/share/edk2/ovmf/OVMF_CODE.fd
>   /usr/share/edk2/ovmf/OVMF_CODE.secboot.fd
> ...
> 
> (
> Learnt this from a discussion with Michal Privoznik in this bug,
> comment#2:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1733940 -- RFE: Report
> firmware (FW) paths in domainCapabilities based on FW descriptor
> files
> )
> 
> Signed-off-by: Kashyap Chamarthy 
> ---
> I only did a cursory check on if I missed to add any other valid paths
> for other architectures.
> ---

For the change:

Reviewed-by: Cole Robinson 

But I'm not sure how much we care now that firmware.repo is in the mix.
I'll leave it up to Michal whether to apply

- Cole



>  libvirt.spec.in | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index b13b863928..99950495a7 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1135,10 +1135,17 @@ exit 1
>  # Nightly edk2.git-arm
>  
> LOADERS="$LOADERS:/usr/share/edk2.git/arm/QEMU_EFI-pflash.raw:/usr/share/edk2.git/arm/vars-template-pflash.raw"
>  
> -# Fedora edk2-ovmf
> +# Fedora edk2-ovmf, x86_64
>  
> LOADERS="$LOADERS:/usr/share/edk2/ovmf/OVMF_CODE.fd:/usr/share/edk2/ovmf/OVMF_VARS.fd"
> +# Fedora edk2-ovmf, x86_64, with Secure Boot
> +
> LOADERS="$LOADERS:/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd:/usr/share/edk2/ovmf/OVMF_VARS.secboot.fd"
>  # Fedora edk2-ovmf-ia32
>  
> LOADERS="$LOADERS:/usr/share/edk2/ovmf-ia32/OVMF_CODE.fd:/usr/share/edk2/ovmf-ia32/OVMF_VARS.fd"
> +# Fedora edk2-ovmf-ia32, with Secure Boot.  (NB: Unlike x86_64, for
> +# 'ia32', there is no secboot-variant "VARS" file (NVRAM template).
> +# So the NVRAM template for 'ovmf-ia32/OVMF_CODE.secboot.fd' is the
> +# same as the one for the non-secboot variant.)
> +
> LOADERS="$LOADERS:/usr/share/edk2/ovmf-ia32/OVMF_CODE.secboot.fd:/usr/share/edk2/ovmf-ia32/OVMF_VARS.fd"
>  # Fedora edk2-aarch64
>  
> LOADERS="$LOADERS:/usr/share/edk2/aarch64/QEMU_EFI-pflash.raw:/usr/share/edk2/aarch64/vars-template-pflash.raw"
>  # Fedora edk2-arm
> 


- Cole

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

Re: [libvirt] Error when creating VM with persistent memory

[Michal Prívozník]

On 9/4/19 6:31 PM, Dan Williams wrote:
> On Wed, Sep 4, 2019 at 12:56 AM Seema Pandit  wrote:
>>
>> Actually there is still some issue around this. When trying to start another 
>> VM, so using even more pmem, there is a different issue now. Error copied 
>> below.
>>
>> []# virsh start manual_clone
>>
>> error: Failed to start domain manual_clone
>>
>> error: internal error: qemu unexpectedly closed the monitor: ftruncate: 
>> Invalid argument
>>
>> 2019-09-03T21:28:41.031924Z qemu-system-x86_64: -object 
>> memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/dev/dax1.1,share=yes,size=7301032:
>>  os_mem_prealloc: Insufficient free host memory pages available to allocate 
>> guest RAM
> 
> /dev/dax instances do not support the ftruncate syscall, and
> device-dax instances are already fully allocated by definition. The
> prealloc option is simply invalid for /dev/dax targets.

Ah, in that case we need an XML knob that if set doesn't put
prealloc=yes onto the qemu cmd line. Unfortunatelly, I don't think I
have any spare time to implement that, so please be my guest.

Michal

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

Re: [libvirt] [PATCH v4 0/8] CPU Model Baseline and Comparison for s390x

[David Hildenbrand]

On 10.09.19 17:22, Jiri Denemark wrote:
> On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:
>> On 8/20/19 10:06 AM, Jiri Denemark wrote:
>>> First, let me apologize for such a late review. I'll try my best to
>>> review your series earlier next time.
>>>
>>
>> Your review is greatly appreciated! I haven't replied to your other
>> posts on this series as my comments were mostly acknowledgements rather 
>> than discussion pieces. I'm working through them.
>>
>>> On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
 When baselining CPU models and the user appends the --features argument 
 to the command, s390x will only report back features that supersede the 
 base model definition.

 *NOTE* if the --features flag is intended to expand /ALL/ features
 available to a CPU model (such as the huge list of features reported
 by a full CPU model expansion), please let me know and I can resolve 
 this.
>>>
>>> I'm not sure how well this fits s390 world, but the semantics of
>>> VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
>>> features which are hidden behind the CPU model. That is, all feature
>>> which you'd get when starting QEMU with just the CPU model name and no
>>> additional features. The extra features should not be touched at all.
>>> Specifically, removing them when the flag is not used is wrong.
>>>
>>> To me this looks like the flag should really result in running
>>> query-cpu-model-expansion (but likely the "static" one rather then
>>> "full" expansion) on the baseline CPU model and reporting the enabled
>>> features along with those already included in the baseline feature set.
>>>
>>
>> Actually, query-cpu-model-baseline will return a CPU model along with a
>> feature set. The features returned are the same as those produced from a 
>> static expansion on the model.
>>
>> Correct me if I am wrong here: virsh should report features *only* if the
>> --features flag is present. Otherwise, we only report the model name (which
>> can be accomplished by stripping the result of all reported features).
> 
> No, this is wrong in general (although it maybe correct for s390, I
> don't know).

David here: Not correct for s390x

> 
> Let me explain with some hypothetical CPU models.
> 
> Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
> Model2 will enable f1, f2, f3, f4, f5.
> Model3 will enable f1, f2, f3, f4, f6.
> 
> Running baseline command for [Model1, Model2, Model3] should return
> Model1 with no additional features as [f1, f2, f3] are in only ones
> common to all three models and they are all enabled by Model1.
> 
> However, baseline of [Model2, Model3] should return Model1 + f4. The
> common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
> f2, f3] only, which means we need to add f4 explicitly.
> 
> With --features, even the features enabled implicitly by a specific CPU
> model should be returned. That is, the first result would be Model1 +
> [f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].
> 
> In other words, stripping all features if no --features option is used
> is wrong. Only features implicitly enabled by the CPU model should be
> stripped.
> 
> Specifically, baseline without --features on the following CPU
> definition
> 
> 
>   Model1
>   
> 
> 
> should be the same definition (i.e., Model1 + f4).

s390x will fallback to base models when baselining/expanding ("minimum
feature set we expect to be around for a certain cpu generation", which
is independent of the QEMU version and will never change). But if you're
already using base models, it works as you would expect it.

E.g.

Model1 is [f1, f2, f3]
Model1-base is [f1, f2]

Model2 is [f1, f2, f3, f4]
Model2-base is [f1, f2, f3]

Baselining Model1 with Model2 would result in
Model1-base + f3

For this, QMP "query-cpu-model-baseline" can be used.

Note: When expending cpu models (e.g. host), one would already always
get base models. So there, it would work completely as you expect it.

Now, to achieve the "--features" part, simply throw the result of
"query-cpu-model-baseline" into "query-cpu-model-expansion" with
"CpuModelExpansionType" == "full"

> 
> The question is whether you can enabled extra features on s390 or you
> can only use plain CPU models with no additional features. If additional
> features are allowed, I'd guess baseline without features should return
> the result of the QMP command minus the features a static expansion of
> the CPU model itself would return (this is assuming f4 will be included
> in the result we get from QEMU for Model1 + f4). When --features is
> used, the result from QEMU should be returned.

We do have plenty of extra cpu features on s390x. *Plenty* :)

> 
> I hope it's clear now. However, I'm not sure how CPU models work on
> s390 and I'd be happy if you could explain it to me. Especially, whether
> something similar to what I described can happen there.
> 
> 

Re: [libvirt] [PATCH v4 0/8] CPU Model Baseline and Comparison for s390x

[Jiri Denemark]

On Tue, Sep 03, 2019 at 15:32:34 -0400, Collin Walling wrote:
> On 8/20/19 10:06 AM, Jiri Denemark wrote:
> > First, let me apologize for such a late review. I'll try my best to
> > review your series earlier next time.
> > 
> 
> Your review is greatly appreciated! I haven't replied to your other
> posts on this series as my comments were mostly acknowledgements rather 
> than discussion pieces. I'm working through them.
> 
> > On Wed, Jul 17, 2019 at 10:03:21 -0400, Collin Walling wrote:
> >> When baselining CPU models and the user appends the --features argument 
> >> to the command, s390x will only report back features that supersede the 
> >> base model definition.
> >>
> >> *NOTE* if the --features flag is intended to expand /ALL/ features
> >> available to a CPU model (such as the huge list of features reported
> >> by a full CPU model expansion), please let me know and I can resolve 
> >> this.
> > 
> > I'm not sure how well this fits s390 world, but the semantics of
> > VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag is to report all CPU
> > features which are hidden behind the CPU model. That is, all feature
> > which you'd get when starting QEMU with just the CPU model name and no
> > additional features. The extra features should not be touched at all.
> > Specifically, removing them when the flag is not used is wrong.
> > 
> > To me this looks like the flag should really result in running
> > query-cpu-model-expansion (but likely the "static" one rather then
> > "full" expansion) on the baseline CPU model and reporting the enabled
> > features along with those already included in the baseline feature set.
> > 
> 
> Actually, query-cpu-model-baseline will return a CPU model along with a
> feature set. The features returned are the same as those produced from a 
> static expansion on the model.
> 
> Correct me if I am wrong here: virsh should report features *only* if the
> --features flag is present. Otherwise, we only report the model name (which
> can be accomplished by stripping the result of all reported features).

No, this is wrong in general (although it maybe correct for s390, I
don't know).

Let me explain with some hypothetical CPU models.

Model1 (when used as -cpu Model1) will enable features f1, f2, f3.
Model2 will enable f1, f2, f3, f4, f5.
Model3 will enable f1, f2, f3, f4, f6.

Running baseline command for [Model1, Model2, Model3] should return
Model1 with no additional features as [f1, f2, f3] are in only ones
common to all three models and they are all enabled by Model1.

However, baseline of [Model2, Model3] should return Model1 + f4. The
common set of features is [f1, f2, f3, f4], but Model1 would enable [f1,
f2, f3] only, which means we need to add f4 explicitly.

With --features, even the features enabled implicitly by a specific CPU
model should be returned. That is, the first result would be Model1 +
[f1, f2, f3] and the second result would be Model1 + [f1, f2, f3, f4].

In other words, stripping all features if no --features option is used
is wrong. Only features implicitly enabled by the CPU model should be
stripped.

Specifically, baseline without --features on the following CPU
definition


  Model1
  


should be the same definition (i.e., Model1 + f4).

The question is whether you can enabled extra features on s390 or you
can only use plain CPU models with no additional features. If additional
features are allowed, I'd guess baseline without features should return
the result of the QMP command minus the features a static expansion of
the CPU model itself would return (this is assuming f4 will be included
in the result we get from QEMU for Model1 + f4). When --features is
used, the result from QEMU should be returned.

I hope it's clear now. However, I'm not sure how CPU models work on
s390 and I'd be happy if you could explain it to me. Especially, whether
something similar to what I described can happen there.

Jirka

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

[libvirt] [PATCH] libvirt_python:fix bug of sanitytest.py script

[ossdev]

From: ossdev 

libvirt-python:fix bug of sanitytest.py script

Signed-off-by: ossdev 
---
 sanitytest.py | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/sanitytest.py b/sanitytest.py
index e87b57d..c5d1f42 100644
--- a/sanitytest.py
+++ b/sanitytest.py
@@ -22,6 +22,21 @@ def get_libvirt_api_xml_path():
 sys.exit(proc.returncode)
 return stdout.splitlines()[0]
 
+def sanitize_enum_val(value):
+if value == 'VIR_TYPED_PARAM_INT':
+value = 1
+elif value == 'VIR_TYPED_PARAM_UINT':
+value = 2
+elif value == 'VIR_TYPED_PARAM_LLONG':
+value = 3
+elif value == 'VIR_TYPED_PARAM_ULLONG':
+value = 4
+elif value == 'VIR_TYPED_PARAM_DOUBLE':
+value = 5
+elif value == 'VIR_TYPED_PARAM_BOOLEAN':
+value = 6
+return value
+
 # Path to the libvirt API XML file
 if len(sys.argv) >= 3:
 xml = sys.argv[2]
@@ -48,8 +63,8 @@ set = tree.xpath('/api/symbols/enum')
 for n in set:
 typ = n.attrib['type']
 name = n.attrib['name']
-val = n.attrib['value']
-
+#val = n.attrib['value']
+val = sanitize_enum_val(n.attrib['value'])
 if typ not in enumvals:
 enumvals[typ] = {}
 
-- 
2.17.1


-- 


*Disclaimer* -The information transmitted is intended solely for the 
individual
or entity to which it is addressed and may contain confidential 
and/or
privileged material. Any review, re-transmission, dissemination or 
other use of
or taking action in reliance upon this information by persons 
or entities other
than the intended recipient is prohibited. If you have 
received this email in
error please contact the sender and delete the 
material from any computer. In
such instances you are further prohibited 
from reproducing, disclosing,
distributing or taking any action in reliance 
on it.As a recipient of this email,
you are responsible for screening its 
contents and the contents of any
attachments for the presence of viruses. 
No liability is accepted for any
damages caused by any virus transmitted by 
this email.

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

[libvirt] [PATCH v5 2/2] snapshot: Store both config and live XML in the snapshot domain

[Jiri Denemark]

From: "Maxiwell S. Garcia" 

The snapshot-create operation of running guests saves the live
XML and uses it to replace the active and inactive domain in
case of revert. So, the config XML is ignored by the snapshot
process. This commit changes it and adds the config XML in the
snapshot XML as the  entry.

In case of offline guest, the behavior remains the same and the
config XML is saved in the snapshot XML as  entry. The
behavior of older snapshots of running guests, that don't have
the new , remains the same too. The revert, in
this case, overrides both active and inactive domain with the
 entry. So, the  in the snapshot XML is
not required to snapshot work, but it's useful to preserve the
config XML of running guests.

Signed-off-by: Maxiwell S. Garcia 
Reviewed-by: Daniel Henrique Barboza 
Tested-by: Daniel Henrique Barboza 
Reviewed-by: Jiri Denemark 
---
 src/conf/moment_conf.c   |  1 +
 src/conf/moment_conf.h   | 11 +++
 src/conf/snapshot_conf.c | 22 --
 src/qemu/qemu_driver.c   | 62 +++-
 4 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c
index fea13f0f97..f54a44b33e 100644
--- a/src/conf/moment_conf.c
+++ b/src/conf/moment_conf.c
@@ -66,6 +66,7 @@ virDomainMomentDefDispose(void *obj)
 VIR_FREE(def->description);
 VIR_FREE(def->parent_name);
 virDomainDefFree(def->dom);
+virDomainDefFree(def->inactiveDom);
 }
 
 /* Provide defaults for creation time and moment name after parsing XML */
diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h
index 9fdbef2172..70cc47bd70 100644
--- a/src/conf/moment_conf.h
+++ b/src/conf/moment_conf.h
@@ -36,7 +36,18 @@ struct _virDomainMomentDef {
 char *parent_name;
 long long creationTime; /* in seconds */
 
+/*
+ * Store the active domain definition in case of online
+ * guest and the inactive domain definition in case of
+ * offline guest
+ */
 virDomainDefPtr dom;
+
+/*
+ * Store the inactive domain definition in case of online
+ * guest and leave NULL in case of offline guest
+ */
+virDomainDefPtr inactiveDom;
 };
 
 virClassPtr virClassForDomainMomentDef(void);
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 7996589ad2..cce9a7999c 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -235,6 +235,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 virDomainSnapshotDefPtr def = NULL;
 virDomainSnapshotDefPtr ret = NULL;
 xmlNodePtr *nodes = NULL;
+xmlNodePtr inactiveDomNode = NULL;
 size_t i;
 int n;
 char *creation = NULL, *state = NULL;
@@ -244,6 +245,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 char *memoryFile = NULL;
 bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE);
 virSaveCookieCallbacksPtr saveCookie = 
virDomainXMLOptionGetSaveCookie(xmlopt);
+int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+  VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
 
 if (!(def = virDomainSnapshotDefNew()))
 return NULL;
@@ -293,8 +296,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
  * clients will have to decide between best effort
  * initialization or outright failure.  */
 if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
-int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
-  VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
 xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
 
 VIR_FREE(tmp);
@@ -311,6 +312,16 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 } else {
 VIR_WARN("parsing older snapshot that lacks domain");
 }
+
+/* /inactiveDomain entry saves the config XML present in a running
+ * VM. In case of absent, leave parent.inactiveDom NULL and use
+ * parent.dom for config and live XML. */
+if ((inactiveDomNode = virXPathNode("./inactiveDomain", ctxt))) {
+def->parent.inactiveDom = virDomainDefParseNode(ctxt->node->doc, 
inactiveDomNode,
+caps, xmlopt, 
NULL, domainflags);
+if (!def->parent.inactiveDom)
+goto cleanup;
+}
 } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, >parent) < 0) 
{
 goto cleanup;
 }
@@ -908,6 +919,13 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf,
 virBufferAddLit(buf, "\n");
 }
 
+if (def->parent.inactiveDom) {
+if (virDomainDefFormatInternalSetRootName(def->parent.inactiveDom, 
caps,
+  domainflags, buf, xmlopt,
+  "inactiveDomain") < 0)
+goto error;
+}
+
 if (virSaveCookieFormatBuf(buf, def->cookie,
virDomainXMLOptionGetSaveCookie(xmlopt)) < 0)
 

Re: [libvirt] [PATCH v4 2/2] snapshot: Store both config and live XML in the snapshot domain

[Jiri Denemark]

On Thu, Aug 29, 2019 at 17:55:43 -0300, Maxiwell S. Garcia wrote:
> The snapshot-create operation of running guests saves the live
> XML and uses it to replace the active and inactive domain in
> case of revert. So, the config XML is ignored by the snapshot
> process. This commit changes it and adds the config XML in the
> snapshot XML as the  entry.
> 
> In case of offline guest, the behavior remains the same and the
> config XML is saved in the snapshot XML as  entry. The
> behavior of older snapshots of running guests, that don't have
> the new , remains the same too. The revert, in
> this case, overrides both active and inactive domain with the
>  entry. So, the  in the snapshot XML is
> not required to snapshot work, but it's useful to preserve the
> config XML of running guests.
> 
> Signed-off-by: Maxiwell S. Garcia 
> ---
>  src/conf/moment_conf.c   |  1 +
>  src/conf/moment_conf.h   | 11 +++
>  src/conf/snapshot_conf.c | 22 --
>  src/qemu/qemu_driver.c   | 37 -
>  4 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c
> index fea13f0f97..f54a44b33e 100644
> --- a/src/conf/moment_conf.c
> +++ b/src/conf/moment_conf.c
> @@ -66,6 +66,7 @@ virDomainMomentDefDispose(void *obj)
>  VIR_FREE(def->description);
>  VIR_FREE(def->parent_name);
>  virDomainDefFree(def->dom);
> +virDomainDefFree(def->inactiveDom);
>  }
>  
>  /* Provide defaults for creation time and moment name after parsing XML */
> diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h
> index 9fdbef2172..70cc47bd70 100644
> --- a/src/conf/moment_conf.h
> +++ b/src/conf/moment_conf.h
> @@ -36,7 +36,18 @@ struct _virDomainMomentDef {
>  char *parent_name;
>  long long creationTime; /* in seconds */
>  
> +/*
> + * Store the active domain definition in case of online
> + * guest and the inactive domain definition in case of
> + * offline guest
> + */
>  virDomainDefPtr dom;
> +
> +/*
> + * Store the inactive domain definition in case of online
> + * guest and leave NULL in case of offline guest
> + */
> +virDomainDefPtr inactiveDom;

OK, I can live with this since dom/inactiveDom are direct equivalents to
domain/inactiveDomain elements in the snapshot XML, ...

>  };
>  
>  virClassPtr virClassForDomainMomentDef(void);
...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 78f5471b79..67511b705a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
...
> @@ -16572,11 +16580,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
> snapshot,
>   * in the failure cases where we know there was no change?  */
>  }
>  
> -/* Prepare to copy the snapshot inactive xml as the config of this
> - * domain.
> - *
> - * XXX Should domain snapshots track live xml rather
> - * than inactive xml?  */
> +/* Prepare to copy the snapshot inactive domain as the config XML
> + * and the snapshot domain as the live XML. In case of inactive domain
> + * NULL, both config and live XML will be copied from snapshot domain.
> + */
>  if (snap->def->dom) {
>  config = virDomainDefCopy(snap->def->dom, caps,
>driver->xmlopt, priv->qemuCaps, true);
> @@ -16584,6 +16591,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
> snapshot,
>  goto endjob;
>  }
>  
> +if (snap->def->inactiveDom) {
> +inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps,
> +  driver->xmlopt, priv->qemuCaps, 
> true);
> +if (!inactiveConfig)
> +goto endjob;
> +} else {
> +inactiveConfig = config;
> +}
> +

...but the code here and the comment above should be changed a bit. You
can't point both config and inactiveConfig to the same virDomainDef
structure (see below). I believe config/inactiveConfig should be used
strictly for active/inactive domain definition (thus config could be
NULL for inactive snapshots) and we should copy the definition twice for
active snapshots without inactiveDom:

  +if (snap->def->inactiveDom) {
  +inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps,
  +  driver->xmlopt, priv->qemuCaps, 
true);
  +if (!inactiveConfig)
  +goto endjob;
  +} else {
  +/* Inactive domain definition is missing:
  + * - either this is an old active snapshot and we need to copy the
  + *   active definition as an inactive one
  + * - or this is an inactive snapshot which means config contains the
  + *   inactive definition.
  + */
  +if (snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
  +snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) {
  +inactiveConfig = virDomainDefCopy(snap->def->dom, 

Re: [libvirt] [PATCH] domain_conf: Drop ancient reservation of unit 7 in drive address

[Ján Tomko]

On Wed, Aug 28, 2019 at 11:52:45AM +0200, Michal Privoznik wrote:

Introduced in v1.0.6~3, the idea was that unit 7 was reserved and
couldn't be used by QEMU. Well, that limitation is long gone.



How does "long gone" translate to our platform support matrix?

Jano


Signed-off-by: Michal Privoznik 
---
src/conf/domain_conf.c | 6 --
1 file changed, 6 deletions(-)



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] domain_conf: Drop ancient reservation of unit 7 in drive address

[Daniel Henrique Barboza]

On 8/28/19 6:52 AM, Michal Privoznik wrote:

Introduced in v1.0.6~3, the idea was that unit 7 was reserved and
couldn't be used by QEMU. Well, that limitation is long gone.

Signed-off-by: Michal Privoznik 
---



Reviewed-by: Daniel Henrique Barboza 



  src/conf/domain_conf.c | 6 --
  1 file changed, 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b7a342bb91..6a019490fd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4812,12 +4812,6 @@ bool
  virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
  const virDomainDeviceDriveAddress *addr)
  {
-/* In current implementation, the maximum unit number of a controller
- * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
- * is 16, the controller itself is on unit 7 */
-if (addr->unit == 7)
-return true;
-
  if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI,
addr) ||
  virDomainDriveAddressIsUsedByHostdev(def,


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

Re: [libvirt] [PATCH 0/5] qemu: Use FW descriptors to report FW image paths

[Michal Privoznik]

On 8/5/19 6:14 PM, Michal Privoznik wrote:
>

Ping.

Michal

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

Re: [libvirt] [PATCH] domain_conf: Drop ancient reservation of unit 7 in drive address

[Michal Privoznik]

On 8/28/19 11:52 AM, Michal Privoznik wrote:
>

Ping.

Michal

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

[libvirt] [PATCH] qemu: Don't leak domain def when RevertToSnapshot fails

[Jiri Denemark]

Once we copy the domain definition from virDomainSnapshotDef, we either
need to assign it to the domain object or free it to avoid memory leaks.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_driver.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b28a26c3d6..a984b1e65c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16568,6 +16568,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 virCPUDefPtr origCPU = NULL;
 unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
 qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
+bool defined = false;
 
 virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
   VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
@@ -16779,6 +16780,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 virDomainObjAssignDef(vm, config, false, NULL);
 virCPUDefFree(priv->origCPU);
 VIR_STEAL_PTR(priv->origCPU, origCPU);
+config = NULL;
+defined = true;
 }
 
 if (cookie && !cookie->slirpHelper)
@@ -16788,8 +16791,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 /* Transitions 2, 3 */
 load:
 was_stopped = true;
-if (config)
+if (config) {
 virDomainObjAssignDef(vm, config, false, NULL);
+config = NULL;
+defined = true;
+}
 
 /* No cookie means libvirt which saved the domain was too old to
  * mess up the CPU definitions.
@@ -16875,8 +16881,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 qemuProcessEndJob(driver, vm);
 goto cleanup;
 }
-if (config)
+if (config) {
 virDomainObjAssignDef(vm, config, false, NULL);
+config = NULL;
+defined = true;
+}
 
 if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
@@ -16944,7 +16953,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 } else if (snap) {
 virDomainSnapshotSetCurrent(vm->snapshots, NULL);
 }
-if (ret == 0 && config && vm->persistent &&
+if (ret == 0 && defined && vm->persistent &&
 !(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
 vm->newDef ? vm->newDef : vm->def))) {
 detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT;
@@ -16960,6 +16969,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 virObjectUnref(cfg);
 virNWFilterUnlockFilterUpdates();
 virCPUDefFree(origCPU);
+virDomainDefFree(config);
 
 return ret;
 }
-- 
2.23.0

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

[libvirt] [PATCH] virnetdevmacvlan: Provide stubs for macvlan related functions

[Michal Privoznik]

In recent commit of 3d21ff72e0e the virNetDevMacVLanTapOpen() and
virNetDevMacVLanTapSetup() functions were exported in our private
symbols. But these functions live in an #ifdef so they need a
stub implementation.
Then in 1b46566ee the virNetDevMacVLanIsMacvtap() function was
implemented but again, only for #idef and without stub.

Signed-off-by: Michal Privoznik 
---

Pushed under build breaker and trivial rules.

 src/util/virnetdevmacvlan.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 9a9750f24a..e8a9b052b6 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -1178,6 +1178,13 @@ int virNetDevMacVLanRestartWithVPortProfile(const char 
*cr_ifname,
 }
 
 #else /* ! WITH_MACVTAP */
+bool virNetDevMacVLanIsMacvtap(const char *ifname ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Cannot create macvlan devices on this platform"));
+return false;
+}
+
 int virNetDevMacVLanCreate(const char *ifname ATTRIBUTE_UNUSED,
const char *type ATTRIBUTE_UNUSED,
const virMacAddr *macaddress ATTRIBUTE_UNUSED,
@@ -1197,6 +1204,26 @@ int virNetDevMacVLanDelete(const char *ifname 
ATTRIBUTE_UNUSED)
 return -1;
 }
 
+int
+virNetDevMacVLanTapOpen(const char *ifname ATTRIBUTE_UNUSED,
+int *tapfd ATTRIBUTE_UNUSED,
+size_t tapfdSize ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Cannot create macvlan devices on this platform"));
+return -1;
+}
+
+int
+virNetDevMacVLanTapSetup(int *tapfd ATTRIBUTE_UNUSED,
+ size_t tapfdSize ATTRIBUTE_UNUSED,
+ bool vnet_hdr ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Cannot create macvlan devices on this platform"));
+return -1;
+}
+
 int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED,
const virMacAddr *macaddress 
ATTRIBUTE_UNUSED,
const char *linkdev 
ATTRIBUTE_UNUSED,
-- 
2.21.0

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

Re: [libvirt] [PATCH 0/2] conf: Avoid checking root element name in virDomainDefParseNode

[Michal Privoznik]

On 9/10/19 10:45 AM, Jiri Denemark wrote:

The only caller for which this check makes sense is virDomainDefParse.
Thus the check should be moved there.

Jiri Denemark (2):
   conf: Add cleanup label to virDomainDefParse
   conf: Avoid checking root element name in virDomainDefParseNode

  src/conf/domain_conf.c | 29 -
  1 file changed, 16 insertions(+), 13 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

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

[libvirt] [PATCH v2 2/2] Revert "qemu: add socket datagram capability"

[Michal Privoznik]

This reverts commit 0cebb6422a63f5a8289ae43a36f8f33eb9956a4c.

This capability is not used anywhere and also it is not contained
in any release so it's safe to just remove it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_capabilities.c  | 5 -
 src/qemu/qemu_capabilities.h  | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml   | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml   | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml  | 1 -
 tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml  | 1 -
 9 files changed, 13 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 136999ad0d..94a8e581df 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -537,7 +537,6 @@ VIR_ENUM_IMPL(virQEMUCaps,
   /* 335 */
   "bochs-display",
   "migration-file-drop-cache",
-  "net-socket-dgram",
   "dbus-vmstate",
 );
 
@@ -4394,10 +4393,6 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCapsPtr qemuCaps)
 ARCH_IS_PPC64(qemuCaps->arch)) {
 virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT);
 }
-
-/* -net socket,fd= with dgram socket (for ex, with slirp helper) */
-if (qemuCaps->version >= 400)
-virQEMUCapsSet(qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM);
 }
 
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 54f91151c6..fe80fb5391 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -518,7 +518,6 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 /* 335 */
 QEMU_CAPS_DEVICE_BOCHS_DISPLAY, /* -device bochs-display */
 QEMU_CAPS_MIGRATION_FILE_DROP_CACHE, /* migration with disk cache on is 
safe for type='file' disks */
-QEMU_CAPS_NET_SOCKET_DGRAM, /* -net socket,fd= with dgram socket */
 QEMU_CAPS_DBUS_VMSTATE, /* -object dbus-vmstate */
 
 QEMU_CAPS_LAST /* this must always be the last item */
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml
index 1627b2cb5e..20f119665b 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml
@@ -165,7 +165,6 @@
   
   
   
-  
   400
   0
   61700758
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
index 73859becab..9ea6f4d046 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
@@ -170,7 +170,6 @@
   
   
   
-  
   400
   0
   42900758
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml
index ab6a774c3f..7503c2dbcd 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml
@@ -168,7 +168,6 @@
   
   
   
-  
   400
   0
   0
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml
index f731e7b4fa..4a94179ee7 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml
@@ -168,7 +168,6 @@
   
   
   
-  
   400
   0
   0
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml
index 6cad000a11..ef802f3d1f 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml
@@ -132,7 +132,6 @@
   
   
   
-  
   400
   0
   39100758
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
index 79593c467a..87c95f4d18 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml
@@ -207,7 +207,6 @@
   
   
   
-  
   400
   0
   43100758
diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml
index 05e6538e37..f4583d7fe7 100644
--- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml
@@ -209,7 +209,6 @@
   
   
   
-  
   450
   0
   43100759
-- 
2.21.0

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

[libvirt] [PATCH v2 1/2] qemu: Enable slirp-helper iff dbus-vmstate present

[Michal Privoznik]

The fact that qemu is capable -netdev socket is not enough to
start a migratable domain. It also needs dbus-vmstate capability.
Since there are already some qemu releases which have
net-socket-dgram capability and don't have dbus-vmstate we need
to check for dbus-vmstate.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_hotplug.c   | 2 +-
 src/qemu/qemu_process.c   | 2 +-
 tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args | 2 +-
 tests/qemuxml2argvtest.c  | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index bd8868b0f7..c31b2928e4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1329,7 +1329,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
 
 case VIR_DOMAIN_NET_TYPE_USER:
 if (!priv->disableSlirp &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM)) {
+virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
 qemuSlirpPtr slirp = qemuInterfacePrepareSlirp(driver, net);
 
 if (!slirp)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 955ba4de4c..0b2afe6841 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5756,7 +5756,7 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver,
 goto cleanup;
 } else if (actualType == VIR_DOMAIN_NET_TYPE_USER &&
!priv->disableSlirp &&
-   virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM)) 
{
+   virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
 qemuSlirpPtr slirp = qemuInterfacePrepareSlirp(driver, net);
 
 QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp;
diff --git a/tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args 
b/tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args
index d51653dcdd..3fb745c212 100644
--- a/tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args
+++ b/tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args
@@ -29,7 +29,7 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
--netdev socket,fd=42,id=hostnet0 \
+-netdev user,id=hostnet0 \
 -device rtl8139,netdev=hostnet0,id=net0,mac=00:11:22:33:44:55,bus=pci.0,\
 addr=0x2 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1f2ae5958a..0773455b98 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -533,7 +533,7 @@ testCompareXMLToArgv(const void *data)
 virDomainNetDefPtr net = vm->def->nets[i];
 
 if (net->type == VIR_DOMAIN_NET_TYPE_USER &&
-virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM)) {
+virQEMUCapsGet(info->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
 qemuSlirpPtr slirp = qemuSlirpNew();
 slirp->fd[0] = 42;
 QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp;
-- 
2.21.0

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

[libvirt] [PATCH v2 0/2] qemu: Enable slirp-helper iff dbus-vmstate present

[Michal Privoznik]

This is technically a v2 of:

https://www.redhat.com/archives/libvir-list/2019-September/msg00281.html

It implements what Jano suggested - use dbus-vmstate capability to
decide if slirp-helper is used.

Michal Prívozník (2):
  qemu: Enable slirp-helper iff dbus-vmstate present
  Revert "qemu: add socket datagram capability"

 src/qemu/qemu_capabilities.c  | 5 -
 src/qemu/qemu_capabilities.h  | 1 -
 src/qemu/qemu_hotplug.c   | 2 +-
 src/qemu/qemu_process.c   | 2 +-
 tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml   | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml   | 1 -
 tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml  | 1 -
 tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml  | 1 -
 tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args | 2 +-
 tests/qemuxml2argvtest.c  | 2 +-
 13 files changed, 4 insertions(+), 17 deletions(-)

-- 
2.21.0

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

Re: [libvirt] [PATCH v4 1/2] qemu: formatting XML from domain def choosing the root name

[Jiri Denemark]

On Thu, Aug 29, 2019 at 17:55:42 -0300, Maxiwell S. Garcia wrote:
> The function virDomainDefFormatInternal() has the predefined root name
> "domain" to format the XML. But to save both active and inactive domain
> in the snapshot XML, the new root name "inactiveDomain" was created.
> So, the new function virDomainDefFormatInternalSetRootName() allows to
> choose the root name of XML. The former function became a tiny wrapper
> to call the new function setting the correct parameters.
> 
> Signed-off-by: Maxiwell S. Garcia 
> ---
>  src/conf/domain_conf.c | 35 ++-
>  src/conf/domain_conf.h |  6 ++
>  2 files changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b7a342bb91..3154c07a86 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -21517,10 +21517,11 @@ virDomainDefParseNode(xmlDocPtr xml,
>  virDomainDefPtr def = NULL;
>  virDomainDefPtr ret = NULL;
>  
> -if (!virXMLNodeNameEqual(root, "domain")) {
> +if ((!virXMLNodeNameEqual(root, "domain")) &&
> +(!virXMLNodeNameEqual(root, "inactiveDomain"))) {
>  virReportError(VIR_ERR_XML_ERROR,
> _("unexpected root element <%s>, "
> - "expecting "),
> + "expecting  or "),
> root->name);
>  goto cleanup;
>  }

The check does not belong to virDomainDefParseNode. It should be moved
to virDomainDefParse, which is the only place we don't select the domain
root element via XPath. Once moved, it must not be changed, since we
don't want to allow  to be allowed in a domain
definition XML files.

I've just sent patches moving this code.

> @@ -28277,17 +28278,29 @@ virDomainDefFormatFeatures(virBufferPtr buf,
>  return virXMLFormatElement(buf, "features", NULL, );
>  }
>  
> -
> -/* This internal version appends to an existing buffer
> - * (possibly with auto-indent), rather than flattening
> - * to string.
> - * Return -1 on failure.  */
>  int
>  virDomainDefFormatInternal(virDomainDefPtr def,
> virCapsPtr caps,
> unsigned int flags,
> virBufferPtr buf,
> virDomainXMLOptionPtr xmlopt)
> +{
> +return virDomainDefFormatInternalSetRootName(def, caps, flags, buf,
> + xmlopt, "domain");
> +}
> +
> +
> +/* This internal version appends to an existing buffer
> + * (possibly with auto-indent), rather than flattening
> + * to string.
> + * Return -1 on failure.  */
> +int
> +virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
> +  virCapsPtr caps,
> +  unsigned int flags,
> +  virBufferPtr buf,
> +  virDomainXMLOptionPtr xmlopt,
> +  const char *rootname)
>  {
>  unsigned char *uuid;
>  char uuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -28312,7 +28325,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  if (def->id == -1)
>  flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
>  
> -virBufferAsprintf(buf, " +if (!rootname || (STRNEQ(rootname, "domain") &&
> +  STRNEQ(rootname, "inactiveDomain")))
> +goto error;

This would return an error without setting any error message. But it's
not a big deal since we don't need runtime checks for programmers'
errors and we can just delete this check completely.

> +
> +virBufferAsprintf(buf, "<%s type='%s'", rootname, type);
>  if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
>  virBufferAsprintf(buf, " id='%d'", def->id);
>  if (def->namespaceData && def->ns.format)
> @@ -28794,7 +28811,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  virDomainSEVDefFormat(buf, def->sev);
>  
>  virBufferAdjustIndent(buf, -2);
> -virBufferAddLit(buf, "\n");
> +virBufferAsprintf(buf, "\n", rootname);
>  
>  if (virBufferCheckError(buf) < 0)
>  goto error;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 33cef5b75c..af1335cc16 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3073,6 +3073,12 @@ int virDomainDefFormatInternal(virDomainDefPtr def,
> unsigned int flags,
> virBufferPtr buf,
> virDomainXMLOptionPtr xmlopt);
> +int virDomainDefFormatInternalSetRootName(virDomainDefPtr def,
> +  virCapsPtr caps,
> +  unsigned int flags,
> +  virBufferPtr buf,
> +  virDomainXMLOptionPtr xmlopt,
> +  const char *rootname);
>  
>  

[libvirt] [PATCH 1/2] conf: Add cleanup label to virDomainDefParse

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/conf/domain_conf.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7f49c8253f..17ddebb575 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21517,16 +21517,18 @@ virDomainDefParse(const char *xmlStr,
   void *parseOpaque,
   unsigned int flags)
 {
-xmlDocPtr xml;
+xmlDocPtr xml = NULL;
 virDomainDefPtr def = NULL;
 int keepBlanksDefault = xmlKeepBlanksDefault(0);
 
-if ((xml = virXMLParse(filename, xmlStr, _("(domain_definition)" {
-def = virDomainDefParseNode(xml, xmlDocGetRootElement(xml), caps,
-xmlopt, parseOpaque, flags);
-xmlFreeDoc(xml);
-}
+if (!(xml = virXMLParse(filename, xmlStr, _("(domain_definition)"
+goto cleanup;
 
+def = virDomainDefParseNode(xml, xmlDocGetRootElement(xml), caps,
+xmlopt, parseOpaque, flags);
+
+ cleanup:
+xmlFreeDoc(xml);
 xmlKeepBlanksDefault(keepBlanksDefault);
 return def;
 }
-- 
2.23.0

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

[libvirt] [PATCH 0/2] conf: Avoid checking root element name in virDomainDefParseNode

[Jiri Denemark]

The only caller for which this check makes sense is virDomainDefParse.
Thus the check should be moved there.

Jiri Denemark (2):
  conf: Add cleanup label to virDomainDefParse
  conf: Avoid checking root element name in virDomainDefParseNode

 src/conf/domain_conf.c | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

-- 
2.23.0

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

[libvirt] [PATCH 2/2] conf: Avoid checking root element name in virDomainDefParseNode

[Jiri Denemark]

The only caller for which this check makes sense is virDomainDefParse.
Thus the check should be moved there.

Signed-off-by: Jiri Denemark 
---
 src/conf/domain_conf.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 17ddebb575..b3b8c543d5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21520,12 +21520,21 @@ virDomainDefParse(const char *xmlStr,
 xmlDocPtr xml = NULL;
 virDomainDefPtr def = NULL;
 int keepBlanksDefault = xmlKeepBlanksDefault(0);
+xmlNodePtr root;
 
 if (!(xml = virXMLParse(filename, xmlStr, _("(domain_definition)"
 goto cleanup;
 
-def = virDomainDefParseNode(xml, xmlDocGetRootElement(xml), caps,
-xmlopt, parseOpaque, flags);
+root = xmlDocGetRootElement(xml);
+if (!virXMLNodeNameEqual(root, "domain")) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unexpected root element <%s>, "
+ "expecting "),
+   root->name);
+goto cleanup;
+}
+
+def = virDomainDefParseNode(xml, root, caps, xmlopt, parseOpaque, flags);
 
  cleanup:
 xmlFreeDoc(xml);
@@ -21566,14 +21575,6 @@ virDomainDefParseNode(xmlDocPtr xml,
 virDomainDefPtr def = NULL;
 virDomainDefPtr ret = NULL;
 
-if (!virXMLNodeNameEqual(root, "domain")) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("unexpected root element <%s>, "
- "expecting "),
-   root->name);
-goto cleanup;
-}
-
 ctxt = xmlXPathNewContext(xml);
 if (ctxt == NULL) {
 virReportOOMError();
-- 
2.23.0

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

Re: [libvirt] [PATCH 5/6] lxcParseConfigString: Don't return success if post parse callback fails

[Pavel Hrdina]

On Mon, Sep 09, 2019 at 06:00:26PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/lxc/lxc_native.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 6/6] lib: Define and use autofree for virConfPtr

[Pavel Hrdina]

On Mon, Sep 09, 2019 at 06:00:27PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/bhyve/bhyve_conf.c| 10 +---
>  src/libvirt-admin.c   |  3 +-
>  src/libvirt.c |  4 +-
>  src/libxl/libxl_conf.c| 22 +++-
>  src/libxl/libxl_driver.c  |  8 +--
>  src/libxl/xen_xl.c| 33 +--
>  src/libxl/xen_xm.c| 19 +++
>  src/locking/lock_daemon_config.c  |  7 +--
>  src/locking/lock_driver_lockd.c   | 18 +++---
>  src/locking/lock_driver_sanlock.c |  3 +-
>  src/logging/log_daemon_config.c   |  7 +--
>  src/lxc/lxc_conf.c| 16 ++
>  src/lxc/lxc_native.c  | 15 ++---
>  src/qemu/qemu_conf.c  | 47 +++-
>  src/remote/remote_daemon_config.c | 14 ++---
>  src/security/security_selinux.c   |  4 +-
>  src/util/virconf.h|  2 +
>  src/vmx/vmx.c |  3 +-
>  tests/virconftest.c   | 93 +--
>  tests/xlconfigtest.c  |  8 +--
>  tests/xmconfigtest.c  |  8 +--
>  tools/virt-login-shell-helper.c   |  3 +-
>  22 files changed, 133 insertions(+), 214 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/6] qemu_conf.c: Fix naming of *AddRemove* functions

[Pavel Hrdina]

On Mon, Sep 09, 2019 at 06:00:22PM +0200, Michal Privoznik wrote:
> Our naming rules prefer qemuObjectOperation() scheme rather than
> qemuOperationObject() for function names. These were not honoured
> in recent commits to qemu_conf.c.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_conf.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/6] qemu_conf: Use more of VIR_AUTOUNREF()

[Pavel Hrdina]

On Mon, Sep 09, 2019 at 06:00:25PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_conf.c | 139 ---
>  1 file changed, 63 insertions(+), 76 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/6] qemu_conf: Drop a pair of needless 'cleanup' labels

[Pavel Hrdina]

On Tue, Sep 10, 2019 at 09:19:50AM +0200, Pavel Hrdina wrote:
> On Mon, Sep 09, 2019 at 06:00:23PM +0200, Michal Privoznik wrote:
> > There are two 'cleanup' labels - one in
> > virQEMUDriverConfigHugeTLBFSInit() and the other in
> > virQEMUDriverConfigSetDefaults() that do nothing more than
> > return and integer value. No memory freeing or anything important
> > is done there. Drop them in favour of returning immediately.
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  src/qemu/qemu_conf.c | 26 --
> >  1 file changed, 8 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index 32d411e536..f11df03cf8 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -402,18 +402,12 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr 
> > hugetlbfs,
> >   const char *path,
> >   bool deflt)
> >  {
> > -int ret = -1;
> > -
> > -if (VIR_STRDUP(hugetlbfs->mnt_dir, path) < 0)
> > -goto cleanup;
> > -
> > -if (virFileGetHugepageSize(path, >size) < 0)
> > -goto cleanup;
> > +if (VIR_STRDUP(hugetlbfs->mnt_dir, path) < 0 ||
> > +virFileGetHugepageSize(path, >size) < 0)
> > +return -1;
> 
> I prefer the old code-style to have two separate if conditions but
> that's just my personal view.
> 
> We don't have a syntax-check rule for that but the body should be
> wrapped in curly braces [1].  I know that our code doesn't follow that
> rule but we should avoid introducing new occurrences.

Ups, missed the link to hacking guide.

[1] 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/6] qemu_conf: Use more of VIR_AUTOFREE()

[Pavel Hrdina]

On Mon, Sep 09, 2019 at 06:00:24PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_conf.c | 130 +--
>  1 file changed, 40 insertions(+), 90 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index f11df03cf8..c3255a6f54 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -181,37 +181,26 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> privileged)
>  virGetGroupID("tss", >swtpm_group) < 0)
>  cfg->swtpm_group = 0; /* fall back to root */
>  } else {
> -char *rundir;
> -char *cachedir;
> +VIR_AUTOFREE(char *) rundir = NULL;
> +VIR_AUTOFREE(char *) cachedir = NULL;
>  
>  cachedir = virGetUserCacheDirectory();
>  if (!cachedir)
>  goto error;
>  
> -if (virAsprintf(>logDir,
> -"%s/qemu/log", cachedir) < 0) {
> -VIR_FREE(cachedir);
> +if (virAsprintf(>logDir, "%s/qemu/log", cachedir) < 0)
>  goto error;
> -}
> -if (virAsprintf(>swtpmLogDir,
> -"%s/qemu/log", cachedir) < 0) {
> -VIR_FREE(cachedir);
> +if (virAsprintf(>swtpmLogDir, "%s/qemu/log", cachedir) < 0)
>  goto error;
> -}
> -if (virAsprintf(>cacheDir, "%s/qemu/cache", cachedir) < 0) {
> -VIR_FREE(cachedir);
> +if (virAsprintf(>cacheDir, "%s/qemu/cache", cachedir) < 0)
>  goto error;
> -}
> -VIR_FREE(cachedir);
>  
>  rundir = virGetUserRuntimeDirectory();
>  if (!rundir)
>  goto error;
>  if (virAsprintf(>stateDir, "%s/qemu/run", rundir) < 0) {
> -VIR_FREE(rundir);
>  goto error;
>  }

This fails syntax-check, please fix before pushing.

> -VIR_FREE(rundir);
>  
>  if (virAsprintf(>swtpmStateDir, "%s/swtpm", cfg->stateDir) < 0)
>  goto error;
> @@ -1261,7 +1250,7 @@ virCapsPtr 
> virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
>  {
>  size_t i, j;
>  virCapsPtr caps;
> -virSecurityManagerPtr *sec_managers = NULL;
> +VIR_AUTOFREE(virSecurityManagerPtr) *sec_managers = NULL;
>  /* Security driver data */
>  const char *doi, *model, *lbl, *type;
>  const int virtTypes[] = {VIR_DOMAIN_VIRT_KVM,
> @@ -1308,12 +1297,10 @@ virCapsPtr 
> virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
>  VIR_DEBUG("Initialized caps for security driver \"%s\" with "
>"DOI \"%s\"", model, doi);
>  }
> -VIR_FREE(sec_managers);
>  
>  return caps;
>  
>   error:
> -VIR_FREE(sec_managers);
>  virObjectUnref(caps);
>  return NULL;
>  }
> @@ -1485,31 +1472,26 @@ qemuCheckUnprivSGIO(virHashTablePtr sharedDevices,
>  const char *device_path,
>  int sgio)
>  {
> -char *sysfs_path = NULL;
> -char *key = NULL;
> +VIR_AUTOFREE(char *) sysfs_path = NULL;
> +VIR_AUTOFREE(char *) key = NULL;
>  int val;
> -int ret = -1;
>  
>  if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL)))
> -goto cleanup;
> +return -1;
>  
>  /* It can't be conflict if unpriv_sgio is not supported by kernel. */
> -if (!virFileExists(sysfs_path)) {
> -ret = 0;
> -goto cleanup;
> -}
> +if (!virFileExists(sysfs_path))
> +return 0;
>  
>  if (!(key = qemuGetSharedDeviceKey(device_path)))
> -goto cleanup;
> +return -1;
>  
>  /* It can't be conflict if no other domain is sharing it. */
> -if (!(virHashLookup(sharedDevices, key))) {
> -ret = 0;
> -goto cleanup;
> -}
> +if (!(virHashLookup(sharedDevices, key)))
> +return 0;
>  
>  if (virGetDeviceUnprivSGIO(device_path, NULL, ) < 0)
> -goto cleanup;
> +return -1;
>  
>  /* Error message on failure needs to be handled in caller
>   * since there is more specific knowledge of device
> @@ -1519,16 +1501,10 @@ qemuCheckUnprivSGIO(virHashTablePtr sharedDevices,
>  sgio == VIR_DOMAIN_DEVICE_SGIO_DEFAULT)) ||
>(val == 1 &&
> sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED))) {
> -ret = -2;
> -goto cleanup;
> +return -2;
>  }
>  
> -ret = 0;
> -
> - cleanup:
> -VIR_FREE(sysfs_path);
> -VIR_FREE(key);
> -return ret;
> +return 0;
>  }
>  
>  
> @@ -1674,7 +1650,7 @@ qemuSharedDiskAddRemoveInternal(virQEMUDriverPtr driver,
>  const char *name,
>  bool addDisk)
>  {
> -char *key = NULL;
> +VIR_AUTOFREE(char *) key = NULL;
>  int ret = -1;
>  
>  if (virStorageSourceIsEmpty(disk->src) ||
> @@ -1701,7 +1677,6 @@ qemuSharedDiskAddRemoveInternal(virQEMUDriverPtr driver,
>  ret = 0;
>   cleanup:
>  

Re: [libvirt] [PATCH 2/6] qemu_conf: Drop a pair of needless 'cleanup' labels

[Pavel Hrdina]

On Mon, Sep 09, 2019 at 06:00:23PM +0200, Michal Privoznik wrote:
> There are two 'cleanup' labels - one in
> virQEMUDriverConfigHugeTLBFSInit() and the other in
> virQEMUDriverConfigSetDefaults() that do nothing more than
> return and integer value. No memory freeing or anything important
> is done there. Drop them in favour of returning immediately.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_conf.c | 26 --
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 32d411e536..f11df03cf8 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -402,18 +402,12 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr 
> hugetlbfs,
>   const char *path,
>   bool deflt)
>  {
> -int ret = -1;
> -
> -if (VIR_STRDUP(hugetlbfs->mnt_dir, path) < 0)
> -goto cleanup;
> -
> -if (virFileGetHugepageSize(path, >size) < 0)
> -goto cleanup;
> +if (VIR_STRDUP(hugetlbfs->mnt_dir, path) < 0 ||
> +virFileGetHugepageSize(path, >size) < 0)
> +return -1;

I prefer the old code-style to have two separate if conditions but
that's just my personal view.

We don't have a syntax-check rule for that but the body should be
wrapped in curly braces [1].  I know that our code doesn't follow that
rule but we should avoid introducing new occurrences.

>  
>  hugetlbfs->deflt = deflt;
> -ret = 0;
> - cleanup:
> -return ret;
> +return 0;
>  }
>  
>  
> @@ -1172,8 +1166,6 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg)
>  int
>  virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg)
>  {
> -int ret = -1;
> -
>  #define SET_TLS_SECRET_UUID_DEFAULT(val) \
>  do { \
>  if (!cfg->val## TLSx509certdir && \
> @@ -1181,7 +1173,7 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr 
> cfg)
>  cfg->defaultTLSx509secretUUID) { \
>  if (VIR_STRDUP(cfg->val## TLSx509secretUUID, \
> cfg->defaultTLSx509secretUUID) < 0) \
> -goto cleanup; \
> +return -1; \
>  } \
>  } while (0)
>  
> @@ -1205,11 +1197,11 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr 
> cfg)
>  if (virFileExists(SYSCONFDIR "/pki/libvirt-"#val)) { \
>  if (VIR_STRDUP(cfg->val ## TLSx509certdir, \
> SYSCONFDIR "/pki/libvirt-"#val) < 0) \
> -goto cleanup; \
> +return -1; \
>  } else { \
>  if (VIR_STRDUP(cfg->val ## TLSx509certdir, \
> cfg->defaultTLSx509certdir) < 0) \
> -goto cleanup; \
> +return -1; \

I would fix all the cases above to use curly-braces but it's a existing
code so I'll leave that up to you.

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list