[libvirt] encountered failed test cases with latest checkout of libvirt

[D L]

Hi all,

This email content might be duplicated with other thread posted in other
places that I do not know yet where to find or search (please let me know
if it is true). I am working on a small bug of virsh domxml-to-native.
Before I made any changes to the code in order to fix the bug, I 'make
 test'-ed and 'valgrind test'-ed the code, encountered four failures in
'make test' and 19 failures in 'make -C tests valgrind'. What I did today
was the following:

git checkout master
git pull
./autogen.sh --prefix=$(pwd)/build
make
make install
# So far so good, no errors ; however

make check VIR_TEST_EXPENSIVE=1
# generated the following test-suite.log
=
   libvirt 3.2.0: tests/test-suite.log
=

# TOTAL: 115
# PASS:  111
# SKIP:  0
# XFAIL: 0
# FAIL:  4
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: virfirewalltest
=

TEST: virfirewalltest
   40  FAIL
FAIL virfirewalltest (exit status: 1)

FAIL: networkxml2firewalltest
=

TEST: networkxml2firewalltest
 0   FAIL
FAIL networkxml2firewalltest (exit status: 1)

FAIL: nwfilterebiptablestest


TEST: nwfilterebiptablestest
 0   FAIL
FAIL nwfilterebiptablestest (exit status: 1)

FAIL: nwfilterxml2firewalltest
==

TEST: nwfilterxml2firewalltest
 0   FAIL
FAIL nwfilterxml2firewalltest (exit status: 1)



make syntax-check VIR_TEST_EXTENSIVE=1
# I did not paste the result here and I do not understand what it means.
# available upon request

Finally,
make -C tests valgrind
# generated file was attached.

It seems most of the valgrind failures were false positives, I can only
tell
by comparing the log file with the examples in the HACKING page. But I
do not have the knowledge to determine.

Could anyone tell me if I am missing something? Having those test failures
is a problem or not? Which ones can be safely ignored? Is there any easier
way to avoid the false positives? I am concerned because I also need to
run the test when I make changes to the code. On the other hand, I would
also like to learn the right way to do the testing or whatever it takes to
make
things right.

I am running linux using vmware Workstation Pro 12 on Windows 10.
Linux version 4.4.55-1-MANJARO (builduser@manjaro) (gcc version 6.3.1
20170306 (GCC) ) #1 SMP PREEMPT



Dan
=
   libvirt 3.2.0: tests/test-suite.log
=

# TOTAL: 115
# PASS:  93
# SKIP:  3
# XFAIL: 0
# FAIL:  19
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: commandtest
=

TEST: commandtest
  ==6856== 32 bytes in 1 blocks are definitely lost in loss record 636 of 743
==6856==at 0x4C2CF35: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6856==by 0x4EA5963: virAlloc (viralloc.c:144)
==6856==by 0x4F18DC0: virThreadCreateFull (virthread.c:228)
==6856==by 0x40468C: mymain (commandtest.c:1242)
==6856==by 0x407F4A: virTestMain (testutils.c:1019)
==6856==by 0x7FD3510: (below main) (in /usr/lib/libc-2.25.so)
==6856== 
.==6857== 8 bytes in 1 blocks are definitely lost in loss record 21 of 744
==6857==at 0x4C2CF35: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6857==by 0x4EA59CC: virAllocN (viralloc.c:191)
==6857==by 0x4EC5A68: virEventPollMakePollFDs (vireventpoll.c:392)
==6857==by 0x4EC5A68: virEventPollRunOnce (vireventpoll.c:631)
==6857==by 0x4EC49C0: virEventRunDefaultImpl (virevent.c:314)
==6857==by 0x404452: virCommandThreadWorker (commandtest.c:1142)
==6857==by 0x4F18A51: virThreadHelper (virthread.c:206)
==6857==by 0x7D9C2E6: start_thread (in /usr/lib/libpthread-2.25.so)
==6857==by 0x809F54E: clone (in /usr/lib/libc-2.25.so)
==6857== 
==6858== 8 bytes in 1 blocks are definitely lost in loss record 21 of 744
==6858==at 0x4C2CF35: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6858==by 0x4EA59CC: virAllocN (viralloc.c:191)
==6858==by 0x4EC5A68: virEventPollMakePollFDs (vireventpoll.c:392)
==6858==by 0x4EC5A68: virEventPollRunOnce (vireventpoll.c:631)
==6858==by 0x4EC49C0: virEventRunDefaultImpl (virevent.c:314)
==6858==by 0x404452: virCommandThreadWorker (commandtest.c:1142)
==6858==by 0x4F18A51: virThreadHelper (virthread.c:206)
==6858==by 0x7D9C2E6: start_thread (in /usr/lib/libpthread-2.25.so)
==6858==by 0x809F54E: clone (in /usr/lib/libc-2.25.so)
==6858== 
...==6862== 8 bytes in 1 blocks are definitely lost in loss record 20 of 743
==6862==at 0x4C2CF35: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6862==by 0x4EA59CC: virAllocN (viralloc.c:191)
==6862==by 0x4EC5A68: virEventPollMakePollFDs (vireventpoll.

Re: [libvirt] [PATCH v4 00/14] Introduce vGPU mdev framework to libvirt

[yonglihe]

On 2017年03月27日 15:42, yonglihe wrote:


Verify Summary:
* the none rooted mode starting a high-privileges VM actually.

The configurations is source generated default value except tls disabled.


1. rooted

virsh define ./libvirt/vgpu-win10.xml
Domain vgpu-win10 defined from ./libvirt/vgpu-win10.xml

ubuntu@z-nuc-11:~/vgpu-meta/libvirt-stage$ virsh start vgpu-win10
2017-03-26 23:28:57.385+: 2886: info : libvirt version: 3.2.0
2017-03-26 23:28:57.385+: 2886: info : hostname: z-nuc-11.maas
2017-03-26 23:28:57.385+: 2886: warning : qemuDomainObjTaint:4155 
: Domain id=1 name='vgpu-win10' 
uuid=916c5c36-0437-11e7-a23d-830ed1295d00 is tainted: high-privileges
2017-03-26 23:28:58.010+: 2886: warning : 
virDomainAuditHostdev:456 : Unexpected hostdev type while encoding 
audit message: 4

Domain vgpu-win10 started


2. None rooted
virsh -c qemu:///session
Welcome to lt-virsh, the virtualization interactive terminal.

virsh # define ./libvirt/vgpu-win10.xml
Domain vgpu-win10 defined from ./libvirt/vgpu-win10.xml

virsh # start vgpu-win10
2017-03-26 23:38:11.220+: 2882: warning : qemuDomainObjTaint:4155 
: Domain id=4 name='vgpu-win10' 
uuid=916c5c36-0437-11e7-a23d-830ed1295d00 is tainted: high-privileges
2017-03-26 23:38:12.356+: 2882: warning : 
virDomainAuditHostdev:456 : Unexpected hostdev type while encoding 
audit message: 4

Domain vgpu-win10 started
Please ignore above none rooted testing result, my fault. the proper 
test given following result:


to successfully starting a non rooted vm, the following operation needed:
1.change the ownership/access right of the mdev corresponding vfio
   sudo chown ubuntu:ubuntu /dev/vfio/0

2. set a correct ulimit -l  for the vm
sudo sh -c "ulimit -l 3074424832 && exec su $LOGNAME"

otherwise, it running into the following error:
virsh # start vgpu-win10
 internal error: Process exited prior to exec: libvirt:  error : cannot 
limit locked memory to 3074424832: Operation not permitted


my testing bed is Ubuntu 14.04, there is a similar bug ever reported:
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1276719

I could not make sure if there is special requirements run virsh 
directly from the source tree using the ./run scripts. fix me.




Yongli He




Regards
Yongli He


since v1:
- new  attribute model introduced which tells libvirt which 
device API

should be considered when auto-assigning guest address
- device_api is properly checked, thus taking the 'model' attribute 
only as a

hint to assign "some" address
- new address type 'mdev' is introduced rather than using plain 
 element,

since the address element is more conveniently extendable.
- the emulated mtty driver now works as well out of the box, so no HW 
needed to

review this series --> let's try it :)
- fixed all the nits from v1

since v2:
- dropped the patch introducing new address type 'mdev' since I added by
mistake and only after that realized that the device address type 
enum is used

for guest addresses only
   --> the mdevs are still identified by address element containing 
an 'uuid'

   attribute, I just dropped the enum
- resolved the driver hostdev list race condition raised by Pavel in 
his review
   --> the device API is now checked every time our internal mdev 
object is
   created as opposed to the previous version where because of the 
model being

   checked separately, the locking issues arose.
- rewrote the docs, reflecting the mdev address type drop change
- squashed all security related stuff into 1 patch, also added 
app-armor bits

- as Pavel suggested, moved most of the mdev-related functions out of
virhostdev.c to virmdev.c
- added a few more test cases
- created a new branch 'mdev-next' on my github (more suitable name 
than a
   strict version number) on 
https://github.com/eskultety/libvirt/commits/mdev-next


since v3:
- 'undo' an accidental squash of virmdev.{c,h} module introduction 
into patch

   4/15 and made it a separate patch again
- squash 5/15 into 4/15 as Pavel suggested
- dropped the NEWS patch, as I've so far got at least 4 merge 
conflicts because

of it when rebasing...I'll add it before the series is ready to be
merged...or I'll forget about it like I usually do and add it later :/

Erik

Erik Skultety (14):
   conf: hostdev: Enforce enum-in-switch compile-time checks
   conf: hostdev: Introduce virDomainHostdevSubsysSCSIClear
   conf: Introduce virDomainHostdevDefPostParse
   util: Introduce new module virmdev
   conf: Introduce new hostdev device type mdev
   security: Enable labeling of vfio mediated devices
   conf: Enable cold-plug of a mediated device
   qemu: Assign PCI addresses for mediated devices as well
   hostdev: Maintain a driver list of active mediated devices
   qemu: cgroup: Adjust cgroups' logic to allow mediated devices
   qemu: Bump the memory locking limit for mdevs as well
   qemu: Format mdevs on qemu command line
   test: Add some test cases for our test suite regarding the mdevs
   docs: Document t

Re: [libvirt] [PATCH 1/2] networkUpdateState: Create virMacMap module more frequently

[Martin Kletzander]

On Tue, Mar 28, 2017 at 11:08:01AM +0200, Michal Privoznik wrote:

On 03/28/2017 02:22 AM, Laine Stump wrote:

On 03/22/2017 10:43 AM, Michal Privoznik wrote:

The virMacMap module is there for dumping [domain, ] pairs into a file so that libvirt_guest NSS module can use
it. Whenever a interface is allocated from network (e.g. on


s/a interface/an interface/



Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a753cd78b..0ea8e2348 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj,
 {
 virNetworkDriverStatePtr driver = opaque;
 dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver);
+char *macMapFile = NULL;
 int ret = -1;

 virObjectLock(obj);
@@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj,
 /* If bridge doesn't exist, then mark it inactive */
 if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1))
 obj->active = 0;
+
+if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge)))
+goto cleanup;
+
+if (!(obj->macmap = virMacMapNew(macMapFile)))
+goto cleanup;
+
 break;



... but from what I can understand, the only differences are:

1) the macMapFile used to be regenerated right after reading the
radvdpidfile (which in most cases doesn't exist because I think most of
the time that same duty is handled by dnsmasq instead), and now it is
regenerated *just before* reading radvdpidfile.


I don't think this is any problem. It's just an ordering issue. Those
two features are orthogonal.



2) it used to be regenerated for any network with a def->bridge (so it
would also happen for "unmanaged" bridge networks, where libvirt just
points to an already-existing bridge), and it is now regenerated only
for networks where libvirt creates and destroys the bridge (and might
have a dnsmasq instance running.


Ah. Is that right? I agree that the code is a bit unclear, but the
following should be true:

1) when a network is being freshly started up, the virMacMap module is
created and stored in the network object if and only if dnsmasq is
spawned (because only then it's us who assigns IP addresses).

2) when an interface is allocated/freed from a network that has the
module, the $br.macs file is updated accordingly. The file is created on
the first interface allocation, and unlinked on network undef.

3) when the daemon restarts, networkUpdateState() is called for every
running network. And if the $br.macs file exists for given network, the
module is created (we are reconstructing the network objects after all)
and the file is parsed to restore the previous state. Note, that the
dnsmasq is not spawned again - it kept running while libvirtd was
restarting.

Now, there are two problems that I see:

a) if there is no interface added in the 2nd step and the libvirt daemon
is restarted, in the 3rd step the file does not exist and thus the code
thinks no virMacMap module was used for the network so it does not
create one. This is obviously a bug.

b) if you want to have the module for your network, you need to shut it
down (and thus cut off your domains from the connectivity) and start
over again. It's very annoying. When implementing this, my reasoning was
that it's better to give no results than partial results, it's better to
not have no $br.macs file than have a file containing just newly
allocated interfaces. Well, I was wrong. I think people don't want to
destroy their network unless really necessary. More so if the network
continues running even when the daemon is killed.

Therefore, what I am suggesting here is to rework step 3) so that the
module is created more frequently. It would solve both problems I'm
describing.



Makes sense to me, however you might want to look at an idea in the
second patch.  Maybe it would help a bit.  Maybe it wouldn't :-/

Anyway, even when I'm not against this, I'll leave ACKing this to Laine.


Michal

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


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

Re: [libvirt] [PATCH 2/2] network: Don't crash on domain destroy

[Martin Kletzander]

On Wed, Mar 22, 2017 at 03:43:17PM +0100, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1434882

Imagine the following scenario:

1) virsh net-start default
2) virsh start myFavouriteDomain
3) virsh net-destroy default
4) virsh destroy myFavouriteDomain

(assuming myFavouriteDomain has an interface from default
network)

Regardless of how unlikely this scenario looks like, we should
not crash. The problem is, on net-destroy in
networkShutdownNetworkVirtual() the virMacMap module is unrefed,
but the stale pointer is kept around. Thus when the domain
destroy procedure comes in, networkReleaseActualDevice() and
subsequently networkMacMgrDel() is called. This function sees the
stale pointer and starts calling the virMacMap module APIs which
work over freed memory.

Signed-off-by: Michal Privoznik 
---
src/network/bridge_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0ea8e2348..3fcdd702e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2488,7 +2488,8 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr 
driver,
if (network->def->bandwidth)
virNetDevBandwidthClear(network->def->bridge);

-virObjectUnref(network->macmap);
+if (!virObjectUnref(network->macmap))
+network->macmap = NULL;



You should set the macmap to NULL unconditionally.  If someone is
holding a reference to it, they can decide when to use it.  And they can
unref it just before you're going to use it.  And you should never use
anything you don't have reference for.  And moreover, the network has
always the only reference, nobody else takes this pointer anywhere.

ACK with that changed.

The macmap could also be part of the network, created when the network
was defined, but it would have a "enabled" boolean which would be
toggled on network start-up.  That way you wouldn't have to deal with
references at all.  They are useless in this case anyway, as mentioned
above.


if (network->radvdPid > 0) {
char *radvdpidbase;
--
2.11.0

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


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

Re: [libvirt] [PATCH] news: Make changes understandable for users

[Martin Kletzander]

On Tue, Mar 28, 2017 at 08:09:02PM +0200, Andrea Bolognani wrote:

On Mon, 2017-03-27 at 22:38 +0200, Jiri Denemark wrote:

> When reading release notes, patch summary is not always the best
> description of what users can expect in new version.  I propose
> changing it slightly so that it describes what exactly happens and
> when.
> 
> However, we do not have to add every single code change to the news
> file, that would be ridiculous and unreadable for users.  If the patch
> subject needs changes like this one, I'm rather tempted to say that
> such changes should not be in the news file at all.  So that would be
> the other way how to fix this.
 
I agree, I think we should change the "Bug fixes" part to list only
important/critical bug fixes. It's not going to be perfect since a bug
importance is a subjective thing, but we could at least make developers
think about it :-)


It's already supposed to work that way. And yes, it's always
going to be subjective and imperfect :)

I guess the criteria for documenting a bug fix in the release
notes would include the impact the bug had, both in the sense
of how many people are likely to be affected, and how easy it
was to run into it; more criteria could be the amount of time
the bug has been present (bugs that have remained unfixed for
years are unlikely to be release notes material), whether or
not it was possible to work around it and just how basic the
feature broken because of it is.

This one looks like it would qualify on several accounts,
but it could also definitely use a description to flesh out
exactly what was changed, something along the lines of

  
Initialize volumes properly when building LVM storage pools
  
  
Volumes containing filesystem signatures need to have it
wiped out before they can be used in an LVM storage pools.
libvirt was unable to wipe out the signature for some
filesystem (such as ext4) but that limitation has now been
addressed.
  



Yeah, that's almost perfect (s/an LVM/LVM/), but I understand that not
everyone wants to come up with such description.  It would not have to
be if it was similarly worded in the commit message.

Anyway, I guess I'm just overreacting to some of these changes.  I'm
sorry for that.  I just feel like we went out of our ways to make
something nicer for the users out there, and start falling back into the
old tracks not long after it.  I compare it to git's release notes [1]
which I always find a) very understandable and b) to the point
(i.e. brief, no fuzzing around, but also missing only information you
can easily find out yourself, e.g. in a man page), but I don't know why
I'm so touchy regarding this subject.

Martin

[1] https://github.com/git/git/blob/master/Documentation/RelNotes/2.12.2.txt


-- 
Andrea Bolognani / Red Hat / Virtualization


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

Re: [libvirt] [PATCH 3/9] Split out virDomainIOMMUDefFormat

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Make adding subelements easier.

No bz reference here

> ---
>  src/conf/domain_conf.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ffc6a68..1245fdd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -23842,6 +23842,15 @@ virDomainDefIothreadShouldFormat(virDomainDefPtr def)
>  }
>  
>  
> +static void
> +virDomainIOMMUDefFormat(virBufferPtr buf,
> +virDomainIOMMUDefPtr iommu)

Could also use const virDomainIOMMUDef *iommu here I think

ACK regardless,

John

> +{
> +virBufferAsprintf(buf, "\n",
> +  virDomainIOMMUModelTypeToString(iommu->model));
> +}
> +
> +
>  /* This internal version appends to an existing buffer
>   * (possibly with auto-indent), rather than flattening
>   * to string.
> @@ -24595,10 +24604,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  goto error;
>  }
>  
> -if (def->iommu) {
> -virBufferAsprintf(buf, "\n",
> -  
> virDomainIOMMUModelTypeToString(def->iommu->model));
> -}
> +if (def->iommu)
> +virDomainIOMMUDefFormat(buf, def->iommu);
>  
>  virBufferAdjustIndent(buf, -2);
>  virBufferAddLit(buf, "\n");
> 

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

Re: [libvirt] [PATCH 9/9] qemu: format caching-mode on iommu command line

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Format the caching-mode option for the intel-iommu device,
> based on its  attribute value.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
>  src/qemu/qemu_capabilities.c  |  2 ++
>  src/qemu/qemu_capabilities.h  |  1 +
>  src/qemu/qemu_command.c   | 10 ++
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml  |  1 +
>  .../qemuxml2argv-intel-iommu-caching.args | 19 
> +++
>  tests/qemuxml2argvtest.c  |  5 +
>  6 files changed, 38 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.args
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e9cc754..6a884d1 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -365,6 +365,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"query-cpu-definitions", /* 250 */
>"kernel-irqchip",
>"intel-iommu-intremap",
> +  "intel-iommu-caching",
>  );

Once merge conflicts are resolved, you'll find this needs the /* 255 */

>  
>  
> @@ -1735,6 +1736,7 @@ static struct virQEMUCapsStringFlags 
> virQEMUCapsObjectPropsUSBNECXHCI[] = {
>  
>  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIntelIOMMU[] = {
>  { "intremap", QEMU_CAPS_INTEL_IOMMU_INTREMAP },
> +{ "caching-mode", QEMU_CAPS_INTEL_IOMMU_CACHING },
>  };
>  
>  /* see documentation for virQEMUCapsQMPSchemaGetByPath for the query format 
> */
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 16db17f..8ab2ee0 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -401,6 +401,7 @@ typedef enum {
>  QEMU_CAPS_QUERY_CPU_DEFINITIONS, /* qmp query-cpu-definitions */
>  QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, /* -machine kernel_irqchip */
>  QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
> +QEMU_CAPS_INTEL_IOMMU_CACHING, /* intel-iommu.caching-mode */

Likewise, a /* 255 */ comment before this next grouping.

>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ddd889d..57d4408 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6662,6 +6662,16 @@ qemuBuildIOMMUCommandLine(virCommandPtr cmd,
>  virBufferAsprintf(&opts, ",intremap=%s",
>
> virTristateSwitchTypeToString(iommu->intremap));
>  }
> +if (iommu->caching) {

Similar to previous - w/r/t VIR_TRISTATE_SWITCH_ABSENT and presence and
setting of the irqchip value.

John

> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_CACHING)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("iommu: caching mode is not supported "
> + "with this QEMU binary"));
> +goto cleanup;
> +}
> +virBufferAsprintf(&opts, ",caching-mode=%s",
> +  virTristateSwitchTypeToString(iommu->caching));
> +}
>  case VIR_DOMAIN_IOMMU_MODEL_LAST:
>  break;
>  }
> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml 
> b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> index 0798b7d..533bbcc 100644
> --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> @@ -207,6 +207,7 @@
>
>
>
> +  
>2008090
>0
> (v2.9.0-rc0-142-g940a8ce)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.args
> new file mode 100644
> index 000..59cb8a1
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.args
> @@ -0,0 +1,19 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest1 \
> +-S \
> +-machine q35,accel=tcg \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-device intel-iommu,intremap=on,caching-mode=on
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 2051af9..44eed34 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2480,6 +2480,11 @@ mymain(void)
>  QEMU_CAPS_MACHINE_KERNEL_IRQCHIP,
>  QEMU_CAPS_INTEL_IOMMU_INTREMAP,
>  QEMU_CAPS_DEVICE_INTEL_IOMMU);
> +DO_TEST("intel-iommu-caching",
> +QEMU_CAPS_MACHINE_OPT,
> +QEMU_CAPS_DEVICE_INTEL_IOMMU,
> +QEMU_CAPS_INTEL

Re: [libvirt] [PATCH 4/9] conf: add to

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Add a new attribute to control interrupt remapping.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
>  docs/formatdomain.html.in  | 22 -
>  docs/schemas/domaincommon.rng  |  9 +
>  src/conf/domain_conf.c | 38 
> +++---
>  src/conf/domain_conf.h |  1 +
>  .../qemuxml2argv-intel-iommu-irqchip.xml   |  4 ++-
>  .../qemuxml2xmlout-intel-iommu-irqchip.xml |  4 ++-
>  6 files changed, 71 insertions(+), 7 deletions(-)
> 

awful name "intremap", but just following QEMU...

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 32a5f18..6b196d4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7285,7 +7285,9 @@ qemu-kvm -net nic,model=? /dev/null
>  
>  ...
>  
> -  
> +  
> +
> +  
>  
>  ...
>  
> @@ -7296,6 +7298,24 @@ qemu-kvm -net nic,model=? /dev/null
>Currently only the intel model is supported.
>  
>
> +  driver
> +  
> +
> +  The driver subelement can be used to configure
> +  additional options:
> +
> +
> +  intremap
> +  
> +
> +  The intremap attribute with possible values
> +  on and off can be used to
> +  turn on interrupt remapping. Since 
> 3.3.0
> +  (QEMU only)
> +
> +  
> +
> +  

If I read patch1 correctly, support of this feature is dependent upon
the 'irqchip' feature being set to (at least, it seems) "split".
Whether "on" would be valid or not is not clear.

In any case, since there is a dependency, it should be listed here and a
doc link/reference to the irqchip feature description?

Since patch7 only adds support for this when there's a "-device
intel-iommu" and not when there's ",iommu=on", there's also another
dependency that needs to be described.

>  
>  
>  Security label
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8ba3902..68f3680 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3889,6 +3889,15 @@
>
>  intel
>
> +  
> +
> +  
> +
> +  
> +
> +  
> +
> +  
>  
>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1245fdd..da0c9f0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13909,12 +13909,16 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode,
>  
>  
>  static virDomainIOMMUDefPtr
> -virDomainIOMMUDefParseXML(xmlNodePtr node)
> +virDomainIOMMUDefParseXML(xmlNodePtr node,
> +  xmlXPathContextPtr ctxt)
>  {
>  virDomainIOMMUDefPtr iommu = NULL, ret = NULL;
> +xmlNodePtr save = ctxt->node;
>  char *tmp = NULL;
>  int val;
>  
> +ctxt->node = node;
> +
>  if (VIR_ALLOC(iommu) < 0)
>  goto cleanup;
>  
> @@ -13931,10 +13935,20 @@ virDomainIOMMUDefParseXML(xmlNodePtr node)
>  
>  iommu->model = val;
>  
> +VIR_FREE(tmp);
> +if ((tmp = virXPathString("string(./driver/@intremap)", ctxt))) {
> +if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
> +virReportError(VIR_ERR_XML_ERROR, _("unknown intremap value: 
> %s"), tmp);
> +goto cleanup;
> +}
> +iommu->intremap = val;
> +}
> +
>  ret = iommu;
>  iommu = NULL;
>  
>   cleanup:
> +ctxt->node = save;
>  VIR_FREE(iommu);
>  VIR_FREE(tmp);
>  return ret;
> @@ -14087,7 +14101,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>  goto error;
>  break;
>  case VIR_DOMAIN_DEVICE_IOMMU:
> -if (!(dev->data.iommu = virDomainIOMMUDefParseXML(node)))
> +if (!(dev->data.iommu = virDomainIOMMUDefParseXML(node, ctxt)))
>  goto error;
>  break;
>  case VIR_DOMAIN_DEVICE_NONE:
> @@ -18202,7 +18216,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>  }
>  
>  if (n > 0) {
> -if (!(def->iommu = virDomainIOMMUDefParseXML(nodes[0])))
> +if (!(def->iommu = virDomainIOMMUDefParseXML(nodes[0], ctxt)))
>  goto error;
>  }
>  VIR_FREE(nodes);
> @@ -23846,8 +23860,24 @@ static void
>  virDomainIOMMUDefFormat(virBufferPtr buf,
>  virDomainIOMMUDefPtr iommu)
>  {
> -virBufferAsprintf(buf, "\n",
> +virBuffer childBuf = VIR_BUFFER_INITIALIZER;
> +
> +virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2);
> +
> +if (iommu->intremap) {

Since intremap is not a boolean or a pointer and while it may seem
superfluous ... I'd like to see != VIR_TRISTATE_SWITCH_ABSENT to make it
more obvious of the compariso

Re: [libvirt] [PATCH 8/9] conf: add caching attribute to iommu device

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Add a new attribute to control the caching mode.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
>  docs/formatdomain.html.in  |  9 +++
>  docs/schemas/domaincommon.rng  |  5 
>  src/conf/domain_conf.c | 23 +++---
>  src/conf/domain_conf.h |  1 +
>  .../qemuxml2argv-intel-iommu-caching.xml   | 28 
> ++
>  .../qemuxml2xmlout-intel-iommu-caching.xml | 28 
> ++
>  tests/qemuxml2xmltest.c|  2 ++
>  7 files changed, 93 insertions(+), 3 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-caching.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 6b196d4..f3c44ae 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7314,6 +7314,15 @@ qemu-kvm -net nic,model=? /dev/null
>(QEMU only)
>  
>
> +  caching
> +  
> +
> +  The caching attribute with possible values
> +  on and off can be used to
> +  turn on the caching mode. Since 
> 3.3.0
> +  (QEMU only)
> +
> +  

Similar comment/concert regarding irqchip setting as from patch4

>  
>
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 68f3680..49adf69 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3896,6 +3896,11 @@
>
>  
>
> +  
> +
> +  
> +
> +  
>  
>
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index da0c9f0..6bcf84e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13944,6 +13944,15 @@ virDomainIOMMUDefParseXML(xmlNodePtr node,
>  iommu->intremap = val;
>  }
>  
> +VIR_FREE(tmp);
> +if ((tmp = virXPathString("string(./driver/@caching)", ctxt))) {
> +if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
> +virReportError(VIR_ERR_XML_ERROR, _("unknown caching value: 
> %s"), tmp);
> +goto cleanup;
> +}
> +iommu->caching = val;
> +}
> +
>  ret = iommu;
>  iommu = NULL;
>  
> @@ -23864,9 +23873,17 @@ virDomainIOMMUDefFormat(virBufferPtr buf,
>  
>  virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2);
>  
> -if (iommu->intremap) {
> -virBufferAsprintf(&childBuf, "\n",
> -  virTristateSwitchTypeToString(iommu->intremap));
> +if (iommu->intremap || iommu->caching) {

Same comment regarding bool/pointer... Not that it really matters
technically, but it would seem to be a more proper comparison.

> +virBufferAddLit(&childBuf, " +if (iommu->intremap) {
> +virBufferAsprintf(&childBuf, " intremap='%s'",
> +  
> virTristateSwitchTypeToString(iommu->intremap));
> +}
> +if (iommu->caching) {
> +virBufferAsprintf(&childBuf, " caching='%s'",
> +  virTristateSwitchTypeToString(iommu->caching));
> +}
> +virBufferAddLit(&childBuf, "/>\n");
>  }
>  
>  virBufferAsprintf(buf, " diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ad8ae2d..af63b51 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2204,6 +2204,7 @@ typedef enum {
>  struct _virDomainIOMMUDef {
>  virDomainIOMMUModel model;
>  virTristateSwitch intremap;
> +virTristateSwitch caching;
>  };
>  /*
>   * Guest VM main configuration
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.xml 
> b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.xml
> new file mode 100644
> index 000..b3ce6b4
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching.xml
> @@ -0,0 +1,28 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219100
> +  219100
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu
> +
> +
> +   function='0x2'/>
> +
> +
> +
> +
> +
> +  

Does it matter if intremap is on or off?  Since this is a different
test, you could go with off or even better not present to test the logic
to add just caching would work...

> +
> +  
> +
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-caching.xml 
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-caching.xml
> new file mode 100644
> index 000..b3ce6b4

Similar comment from patch1 regarding this being a link.


John
> --- /dev/null
> ++

Re: [libvirt] [PATCH 1/9] conf: add to

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Add a new  element with a mode attribute.
> 
> Possible values are off, split or on.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005

Shouldn't this just go on the "last" patch in the series. IDC really,
but if you're going to add to all patches, then all patches should have
it listed.

> ---
>  docs/formatdomain.html.in  | 10 +++
>  docs/schemas/domaincommon.rng  | 16 ++
>  src/conf/domain_conf.c | 35 
> +-
>  src/conf/domain_conf.h | 13 
>  .../qemuxml2argv-intel-iommu-irqchip.xml   | 29 ++
>  .../qemuxml2xmlout-intel-iommu-irqchip.xml | 29 ++
>  tests/qemuxml2xmltest.c|  2 ++
>  7 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4a3123e..32a5f18 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1627,6 +1627,7 @@
>
>
>
> +  
>  
>  
>  ...
> @@ -1788,6 +1789,15 @@
>for hypervisor to decide.
>Since 2.1.0
>
> +  irqchip
> +  Tune the in-kernel irqchip. Possible values for the
> +  mode attribute are:
> +  on, split and off.
> +  split is useful for using interrupt remapping
> +  with the IOMMU device.
> +  The default is left for hypervisor to decide.
> +  Since 3.3.0 (QEMU only)
> +  

Did you somehow know this wouldn't be reviewed in time for 3.2.0?  ;-)

Once I got to patch 4 & 7, I began to wonder if support there was
dependence upon the value being "split" or is "on" also acceptable. It
would seem "off" and not present wouldn't allow intremap or caching to
work.

At the very least whatever it is that allows the -device intel-iommu to
be present in QEMU 2.7 (if I read virQEMUCapsInitQMPMonitor correctly)
instead of ",iommu=on" would also seem to be a requirement. I know we
don't want to put versions there, but whatever it is that is required
should be listed (at least while it's still fresh in your mind).

>  
>  
>  Time keeping
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index fbedc9b..8ba3902 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4489,6 +4489,9 @@
>
>  
>
> +  
> +
> +  
>  
>
>  
> @@ -4664,6 +4667,19 @@
>  
>
>  
> +  
> +
> +  
> +

should there be a "default" (or undefined - see below) to match the enum
impl list?  or just left empty since there's a features tristate set on
parse (and should be used to control whether it gets formatted).

> +  off
> +  split
> +  on
> +
> +  
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6bbc6a2..ffc6a68 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
>"pmu",
>"vmport",
>"gic",
> -  "smm")
> +  "smm",
> +  "irqchip")
>  
>  VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, 
> VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
>"default",
> @@ -855,6 +856,13 @@ VIR_ENUM_IMPL(virDomainLoader,
>"rom",
>"pflash")
>  
> +VIR_ENUM_IMPL(virDomainIRQChip,
> +  VIR_DOMAIN_IRQCHIP_LAST,
> +  "",

Should this be "undefined"? Or just go away since there's a tristate to
manage the parse and (not yet) format.

> +  "off",
> +  "split",
> +  "on")
> +
>  /* Internal mapping: subset of block job types that can be present in
>   *  XML (remaining types are not two-phase). */
>  VIR_ENUM_DECL(virDomainBlockJob)
> @@ -17278,6 +17286,24 @@ virDomainDefParseXML(xmlDocPtr xml,
>  ctxt->node = node;
>  break;
>  
> +case VIR_DOMAIN_FEATURE_IRQCHIP:
> +node = ctxt->node;
> +ctxt->node = nodes[i];
> +tmp = virXPathString("string(./@mode)", ctxt);
> +if (tmp) {
> +int value = virDomainIRQChipTypeFromString(tmp);
> +if (value < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Unknown irqchip mode: %s"),
> +   tmp);
> +goto error;
> +}
> +

Re: [libvirt] [PATCH 7/9] qemu: format intremap= on intel-iommu command line

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Add the intremap= option to QEMU command line, corresponding
> to the  attribute of the iommu device.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
>  src/qemu/qemu_capabilities.c   |  8 
>  src/qemu/qemu_capabilities.h   |  1 +
>  src/qemu/qemu_command.c| 11 +
>  .../qemucapabilitiesdata/caps_2.4.0.x86_64.replies | 22 +++---
>  .../qemucapabilitiesdata/caps_2.5.0.x86_64.replies | 24 +++
>  .../qemucapabilitiesdata/caps_2.6.0.x86_64.replies | 24 +++
>  .../qemucapabilitiesdata/caps_2.7.0.x86_64.replies | 28 +
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 +
>  .../qemucapabilitiesdata/caps_2.8.0.x86_64.replies | 37 
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 +
>  .../qemucapabilitiesdata/caps_2.9.0.x86_64.replies | 49 
> ++
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 +
>  .../qemuxml2argv-intel-iommu-irqchip.args  |  2 +-
>  tests/qemuxml2argvtest.c   |  1 +
>  14 files changed, 166 insertions(+), 44 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 278badf..e9cc754 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -364,6 +364,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>  
>"query-cpu-definitions", /* 250 */
>"kernel-irqchip",
> +  "intel-iommu-intremap",
>  );
>  
>  
> @@ -1732,6 +1733,10 @@ static struct virQEMUCapsStringFlags 
> virQEMUCapsObjectPropsUSBNECXHCI[] = {
>  { "p3", QEMU_CAPS_NEC_USB_XHCI_PORTS },
>  };
>  
> +static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIntelIOMMU[] = {
> +{ "intremap", QEMU_CAPS_INTEL_IOMMU_INTREMAP },
> +};
> +
>  /* see documentation for virQEMUCapsQMPSchemaGetByPath for the query format 
> */
>  static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
>  { "blockdev-add/arg-type/options/+gluster/debug-level", 
> QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
> @@ -1839,6 +1844,9 @@ static struct virQEMUCapsObjectTypeProps 
> virQEMUCapsObjectProps[] = {
>  { "nec-usb-xhci", virQEMUCapsObjectPropsUSBNECXHCI,
>ARRAY_CARDINALITY(virQEMUCapsObjectPropsUSBNECXHCI),
>-1 },
> +{ "intel-iommu", virQEMUCapsObjectPropsIntelIOMMU,
> +  ARRAY_CARDINALITY(virQEMUCapsObjectPropsIntelIOMMU),
> +  QEMU_CAPS_DEVICE_INTEL_IOMMU},
>  };
>  
>  struct virQEMUCapsPropTypeObjects {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index b9efab8..16db17f 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -400,6 +400,7 @@ typedef enum {
>  /* 250 */
>  QEMU_CAPS_QUERY_CPU_DEFINITIONS, /* qmp query-cpu-definitions */
>  QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, /* -machine kernel_irqchip */
> +QEMU_CAPS_INTEL_IOMMU_INTREMAP, /* intel-iommu.intremap */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c1c7f1a..ddd889d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6652,6 +6652,16 @@ qemuBuildIOMMUCommandLine(virCommandPtr cmd,
>  return -1;
>  }
>  virBufferAddLit(&opts, "intel-iommu");
> +if (iommu->intremap) {

Since it's not a pointer, use VIR_TRISTATE_SWITCH_ABSENT

Also what if irqchip isn't at least at split, what happens?  IOW:
Shouldn't there a check for the feature being available before using?
What happens if this is "on", but the irqchip isn't set?

Thus something that's checking if the feature bit is set and that the
irqchip is at least split or on before adding.

John

> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_INTEL_IOMMU_INTREMAP)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("iommu: interrupt remapping is not 
> supported "
> + "with this QEMU binary"));
> +goto cleanup;
> +}
> +virBufferAsprintf(&opts, ",intremap=%s",
> +  
> virTristateSwitchTypeToString(iommu->intremap));
> +}
>  case VIR_DOMAIN_IOMMU_MODEL_LAST:
>  break;
>  }
> @@ -6659,6 +6669,7 @@ qemuBuildIOMMUCommandLine(virCommandPtr cmd,
>  virCommandAddArgBuffer(cmd, &opts);
>  
>  ret = 0;
> + cleanup:
>  virBufferFreeAndReset(&opts);
>  return ret;
>  }

[...]

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.args 
> b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.args
> index ebf9c3e..d704ee4 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.args
> @@ -16,4 +16,4 @@ QEMU_AUDI

Re: [libvirt] [PATCH 6/9] qemu: refactor qemuBuildIOMMUCommandLine

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Introduce a separate buffer for options and use a helper
> variable for def->iommu.

No bz reference here

> ---
>  src/qemu/qemu_command.c | 21 +++--
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH 5/9] qemu: allow conditional device property probing

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Do not probe for devices that QEMU does not know
> when probing for device options.

No bz reference here

> ---
>  src/qemu/qemu_capabilities.c | 99 
> ++--
>  1 file changed, 68 insertions(+), 31 deletions(-)
> 

ACK

John

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

Re: [libvirt] [PATCH 2/9] qemu: format kernel_irqchip on the command line

[John Ferlan]

On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Add kernel_irqchip=off/split/on to the QEMU command line
> and a capability that looks for it in query-command-line-options
> output.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
>  src/libvirt_private.syms  |  1 +
>  src/qemu/qemu_capabilities.c  |  2 ++
>  src/qemu/qemu_capabilities.h  |  1 +
>  src/qemu/qemu_command.c   | 11 +++
>  tests/qemucapabilitiesdata/caps_1.5.3.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_1.6.0.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_1.7.0.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_2.1.1.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml  |  1 +
>  .../qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml |  1 +
>  .../qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml |  1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml |  1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml   |  1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml   |  1 +
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml  |  1 +
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml  |  1 +
>  .../qemuxml2argv-intel-iommu-irqchip.args | 19 
> +++
>  tests/qemuxml2argvtest.c  |  4 
>  21 files changed, 53 insertions(+)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.args
> 

You'll have merge conflicts here to resolve - something I'm sure you
already know.

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a1c7624..d14b81a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -375,6 +375,7 @@ virDomainIOThreadIDAdd;
>  virDomainIOThreadIDDefFree;
>  virDomainIOThreadIDDel;
>  virDomainIOThreadIDFind;
> +virDomainIRQChipTypeToString;
>  virDomainKeyWrapCipherNameTypeFromString;
>  virDomainKeyWrapCipherNameTypeToString;
>  virDomainLeaseDefFree;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 60ed31e..05d0a91 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -363,6 +363,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>"pcie-root-port",
>  
>"query-cpu-definitions", /* 250 */
> +  "kernel-irqchip",
>  );
>  
>  
> @@ -2986,6 +2987,7 @@ static struct virQEMUCapsCommandLineProps 
> virQEMUCapsCommandLine[] = {
>  { "drive", "throttling.bps-total-max-length", 
> QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH },
>  { "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP },
>  { "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE },
> +{ "machine", "kernel_irqchip", QEMU_CAPS_MACHINE_KERNEL_IRQCHIP },
>  };
>  
>  static int
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 55777f9..b9efab8 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -399,6 +399,7 @@ typedef enum {
>  
>  /* 250 */
>  QEMU_CAPS_QUERY_CPU_DEFINITIONS, /* qmp query-cpu-definitions */
> +QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, /* -machine kernel_irqchip */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2045c2e..58af585 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7316,6 +7316,17 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>  }
>  }
>  
> +if (def->irqchip) {

There's a relationship between irqchip setting and the features tristate
that I think should be used here to remove the need for checking
def->irqchip (and keeping a default/undefined value - from patch1).

I think the GIC example a few lines above provides an example.

Additionally does this code need to check if QEMU_CAPS_MACHINE_IOMMU was
not set or preferably that QEMU_CAPS_DEVICE_INTEL_IOMMU is set in order
to ensure the -device intel-iommu will be present?

It's my assumption from reading patch1 html.in and the reading the
comments in virQEMUCapsInitQMPMonitor regarding iommu that having
",iommu=on" may not work.  It gets even more confusing in subsequent
patches for intremap (support starting w/ qemu 2.4) and caching (support
starting with qemu 2.9).

Whether you drop a comment here or the commit message regarding what's
supported could help someone in the future trying to figure this out.


> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)) 
> {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("kernel-irqchip option is not supported 

Re: [libvirt] [PATCH] news: Make changes understandable for users

[Andrea Bolognani]

On Mon, 2017-03-27 at 22:38 +0200, Jiri Denemark wrote:
> > When reading release notes, patch summary is not always the best
> > description of what users can expect in new version.  I propose
> > changing it slightly so that it describes what exactly happens and
> > when.
> > 
> > However, we do not have to add every single code change to the news
> > file, that would be ridiculous and unreadable for users.  If the patch
> > subject needs changes like this one, I'm rather tempted to say that
> > such changes should not be in the news file at all.  So that would be
> > the other way how to fix this.
> 
> I agree, I think we should change the "Bug fixes" part to list only
> important/critical bug fixes. It's not going to be perfect since a bug
> importance is a subjective thing, but we could at least make developers
> think about it :-)

It's already supposed to work that way. And yes, it's always
going to be subjective and imperfect :)

I guess the criteria for documenting a bug fix in the release
notes would include the impact the bug had, both in the sense
of how many people are likely to be affected, and how easy it
was to run into it; more criteria could be the amount of time
the bug has been present (bugs that have remained unfixed for
years are unlikely to be release notes material), whether or
not it was possible to work around it and just how basic the
feature broken because of it is.

This one looks like it would qualify on several accounts,
but it could also definitely use a description to flesh out
exactly what was changed, something along the lines of

  
Initialize volumes properly when building LVM storage pools
  
  
Volumes containing filesystem signatures need to have it
wiped out before they can be used in an LVM storage pools.
libvirt was unable to wipe out the signature for some
filesystem (such as ext4) but that limitation has now been
addressed.
  

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 8/8] docs: Improve documentation related to memory locking

[Andrea Bolognani]

On Tue, 2017-03-28 at 12:40 -0400, Luiz Capitulino wrote:
> > The strong wording is intentional: we really, really don't
> > want people to enable this unless their setup can't work
> > without it.
> 
> I don't see how using strong wording is going to be helpful
> to users who need to use the feature.
> 
> The documentation should be helpful and technically clear. It
> should instruct, not cause fear.
> 
> Your last paragraph is an improvement, but I really think this
> mindset against  is very wrong.

I believe the current wording strikes a good balance
between informing users who really need the feature about
the potential risks involved and mitigation options that
are available to them, and dissuading people who don't
actually need memory locking from messing with it.

Based on that, I'm not going to be spending any more time
on it, but you're of course welcome to improve it further
and post the resulting patches on the list :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 8/8] docs: Improve documentation related to memory locking

[Martin Kletzander]

On Tue, Mar 28, 2017 at 12:40:04PM -0400, Luiz Capitulino wrote:

On Tue, 28 Mar 2017 18:26:12 +0200
Andrea Bolognani  wrote:


On Tue, 2017-03-28 at 09:43 -0400, Luiz Capitulino wrote:
> > Another stab at it (which plugs into my original version):
> > 
> >   [...] remove the limit on locked memory altogether. Thus,
> >   enabling this option opens up to a potential security risk:
> >   the host will be unable to reclaim the locked memory back
> >   from the guest when it's running out of memory, which means
> >   a malicious guest allocating large amounts of locked memory
> >   could cause a denial-of-service attach on the host. Because
> >   of this, using the option is discouraged unless your [...]
> > 
> > Does it look reasonable?
> 
> That looks fine, although I'd drop "discouraged" because that's
> not helpful to those who must use the feature. I think it's better
> to objectively explain what the problems are and how to prevent or
> mitigate them. That's what I tried to do in my paragraph.

The strong wording is intentional: we really, really don't
want people to enable this unless their setup can't work
without it.


I don't see how using strong wording is going to be helpful
to users who need to use the feature.

The documentation should be helpful and technically clear. It
should instruct, not cause fear.

Your last paragraph is an improvement, but I really think this
mindset against  is very wrong.



I must say I'm kinda on Luiz's side here.  It is clear what everything
does and how does it work and what are the risks.  The discouragement is
subjective *and* redundant as all the risks and technicalities are
explained.  If, for some admin/developer, "your machine may crash" is
not enough, but "your machine may crash, you should not do this" will
make everything rainbows and sunshine, then such person should rather be
user of its own system and not anything else.


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


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

Re: [libvirt] [PATCH 8/8] docs: Improve documentation related to memory locking

[Luiz Capitulino]

On Tue, 28 Mar 2017 18:26:12 +0200
Andrea Bolognani  wrote:

> On Tue, 2017-03-28 at 09:43 -0400, Luiz Capitulino wrote:
> > > Another stab at it (which plugs into my original version):
> > > 
> > >   [...] remove the limit on locked memory altogether. Thus,
> > >   enabling this option opens up to a potential security risk:
> > >   the host will be unable to reclaim the locked memory back
> > >   from the guest when it's running out of memory, which means
> > >   a malicious guest allocating large amounts of locked memory
> > >   could cause a denial-of-service attach on the host. Because
> > >   of this, using the option is discouraged unless your [...]
> > > 
> > > Does it look reasonable?  
> > 
> > That looks fine, although I'd drop "discouraged" because that's
> > not helpful to those who must use the feature. I think it's better
> > to objectively explain what the problems are and how to prevent or
> > mitigate them. That's what I tried to do in my paragraph.  
> 
> The strong wording is intentional: we really, really don't
> want people to enable this unless their setup can't work
> without it.

I don't see how using strong wording is going to be helpful
to users who need to use the feature.

The documentation should be helpful and technically clear. It
should instruct, not cause fear.

Your last paragraph is an improvement, but I really think this
mindset against  is very wrong.

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

Re: [libvirt] [PATCH 1/8] Revert "qemu: Forbid without "

[Andrea Bolognani]

On Tue, 2017-03-28 at 09:27 -0400, Luiz Capitulino wrote:
> > Well, it *does* work if you set it up properly, eg. raise the
> > memory locking limit for the user under which libvirtd will
> > run instead of the user under which QEMU processes will run.
> 
> Doesn't libvirtd run as root?

Yes. You can set the memory locking limit to whatever you
want for the root user, and then QEMU processes spawned by
libvirtd will inherit it.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 8/8] docs: Improve documentation related to memory locking

[Andrea Bolognani]

On Tue, 2017-03-28 at 09:43 -0400, Luiz Capitulino wrote:
> > Another stab at it (which plugs into my original version):
> > 
> >   [...] remove the limit on locked memory altogether. Thus,
> >   enabling this option opens up to a potential security risk:
> >   the host will be unable to reclaim the locked memory back
> >   from the guest when it's running out of memory, which means
> >   a malicious guest allocating large amounts of locked memory
> >   could cause a denial-of-service attach on the host. Because
> >   of this, using the option is discouraged unless your [...]
> > 
> > Does it look reasonable?
> 
> That looks fine, although I'd drop "discouraged" because that's
> not helpful to those who must use the feature. I think it's better
> to objectively explain what the problems are and how to prevent or
> mitigate them. That's what I tried to do in my paragraph.

The strong wording is intentional: we really, really don't
want people to enable this unless their setup can't work
without it.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/2] qemuDomainGetStats: Copy domain ID too

[Richard W.M. Jones]

On Tue, Mar 28, 2017 at 05:15:07PM +0200, Michal Privoznik wrote:
> One of the problems with our virGetDomain function is that it
> copies just domain name and domain UUID. Therefore it's very
> easy to forget aboud domain ID. This can cause some bugs, like
> virConnectGetAllDomainStats not reporting proper domain IDs.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d5fc00e..40c2eab 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19556,6 +19556,9 @@ qemuDomainGetStats(virConnectPtr conn,
>  if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))
>  goto cleanup;
>  
> +/* We have to copy the domain ID by hand */
> +tmp->dom->id = dom->def->id;
> +
>  *record = tmp;
>  tmp = NULL;
>  ret = 0;

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

Re: [libvirt] [PATCH 2/2] virGetDomain: Set domain ID too

[Richard W.M. Jones]

On Tue, Mar 28, 2017 at 05:15:08PM +0200, Michal Privoznik wrote:
> So far our code is full of the following pattern:
> 
>   dom = virGetDomain(conn, name, uuid)
>   if (dom)
>   dom->id = 42;
> 
> There is no reasong why it couldn't be just:
> 
>   dom = virGetDomain(conn, name, uuid, id);
> 
> After all, client domain representation consists of tuple (name,
> uuid, id).
> 
> Signed-off-by: Michal Privoznik 

ACK - looks like a sensible change to improve the static
safety of the code.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

[libvirt] [PATCH 2/2] virGetDomain: Set domain ID too

[Michal Privoznik]

So far our code is full of the following pattern:

  dom = virGetDomain(conn, name, uuid)
  if (dom)
  dom->id = 42;

There is no reasong why it couldn't be just:

  dom = virGetDomain(conn, name, uuid, id);

After all, client domain representation consists of tuple (name,
uuid, id).

Signed-off-by: Michal Privoznik 
---
 daemon/remote.c |  5 +
 src/bhyve/bhyve_driver.c| 20 +-
 src/conf/domain_event.c | 11 +-
 src/conf/virdomainobjlist.c | 10 +++--
 src/datatypes.c |  8 ++--
 src/datatypes.h |  3 ++-
 src/esx/esx_driver.c| 49 ++---
 src/hyperv/hyperv_wmi.c | 15 +-
 src/libxl/libxl_driver.c| 20 +-
 src/libxl/libxl_migration.c |  2 +-
 src/lxc/lxc_driver.c| 20 +-
 src/openvz/openvz_driver.c  | 24 ++
 src/phyp/phyp_driver.c  | 12 +++
 src/qemu/qemu_driver.c  | 26 
 src/qemu/qemu_migration.c   |  4 ++--
 src/remote/remote_driver.c  |  5 +
 src/test/test_driver.c  | 20 +-
 src/uml/uml_driver.c| 15 +-
 src/vbox/vbox_common.c  | 44 +++-
 src/vmware/vmware_driver.c  | 20 +-
 src/vz/vz_driver.c  | 22 ++--
 src/xen/xen_driver.c| 28 ++
 src/xenapi/xenapi_driver.c  | 30 +++
 23 files changed, 129 insertions(+), 284 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 5696b43..1610fea 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -6964,13 +6964,10 @@ remoteDispatchStorageVolGetInfoFlags(virNetServerPtr 
server ATTRIBUTE_UNUSED,
 static virDomainPtr
 get_nonnull_domain(virConnectPtr conn, remote_nonnull_domain domain)
 {
-virDomainPtr dom;
-dom = virGetDomain(conn, domain.name, BAD_CAST domain.uuid);
 /* Should we believe the domain.id sent by the client?  Maybe
  * this should be a check rather than an assignment? XXX
  */
-if (dom) dom->id = domain.id;
-return dom;
+return virGetDomain(conn, domain.name, BAD_CAST domain.uuid, domain.id);
 }
 
 static virNetworkPtr
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 74cc5ab..ed2221a 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -570,9 +570,7 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flag
   VIR_DOMAIN_EVENT_DEFINED_ADDED :
   
VIR_DOMAIN_EVENT_DEFINED_UPDATED);
 
-dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
-if (dom)
-dom->id = vm->def->id;
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
 
  cleanup:
 virObjectUnref(caps);
@@ -817,9 +815,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn,
 if (virDomainLookupByUUIDEnsureACL(conn, vm->def) < 0)
 goto cleanup;
 
-dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
-if (dom)
-dom->id = vm->def->id;
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
 
  cleanup:
 if (vm)
@@ -845,9 +841,7 @@ static virDomainPtr bhyveDomainLookupByName(virConnectPtr 
conn,
 if (virDomainLookupByNameEnsureACL(conn, vm->def) < 0)
 goto cleanup;
 
-dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
-if (dom)
-dom->id = vm->def->id;
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
 
  cleanup:
 virDomainObjEndAPI(&vm);
@@ -873,9 +867,7 @@ bhyveDomainLookupByID(virConnectPtr conn,
 if (virDomainLookupByIDEnsureACL(conn, vm->def) < 0)
 goto cleanup;
 
-dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
-if (dom)
-dom->id = vm->def->id;
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
 
  cleanup:
 if (vm)
@@ -991,9 +983,7 @@ bhyveDomainCreateXML(virConnectPtr conn,
   VIR_DOMAIN_EVENT_STARTED,
   VIR_DOMAIN_EVENT_STARTED_BOOTED);
 
-dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
-if (dom)
-dom->id = vm->def->id;
+dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
 
  cleanup:
 virObjectUnref(caps);
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 6243b42..c15f32a 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -1764,10 +1764,11 @@ virDomainEventDispatchDefaultFunc(virConnectPtr conn,
   virConnectObjectEventGenericCallback cb,
   void *cbopaque)
 {
-virDomainPtr dom = virGetDomain(conn, event->meta.name, event->meta.uuid);
+virDomainPtr dom = virGetDomain(conn, event->meta.name,
+event-

[libvirt] [PATCH 1/2] qemuDomainGetStats: Copy domain ID too

[Michal Privoznik]

One of the problems with our virGetDomain function is that it
copies just domain name and domain UUID. Therefore it's very
easy to forget aboud domain ID. This can cause some bugs, like
virConnectGetAllDomainStats not reporting proper domain IDs.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d5fc00e..40c2eab 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19556,6 +19556,9 @@ qemuDomainGetStats(virConnectPtr conn,
 if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))
 goto cleanup;
 
+/* We have to copy the domain ID by hand */
+tmp->dom->id = dom->def->id;
+
 *record = tmp;
 tmp = NULL;
 ret = 0;
-- 
2.10.2

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

[libvirt] [PATCH 0/2] Slightly rework domain ID handling

[Michal Privoznik]

The first patch fixes an actual bug: our virConnectGetAllDomainStats API does
not fill out the domain ID, therefore users just see domain name and UUID.

And while fixing it I realized, that our virGetDomain function can do better.

However, given that we are in the freeze, I'd like to push just the first patch
and save the second for after the release as it touches a lot of places and
thus has a great potential o breaking things. Although, in my testing it
actually fixed some other places that we forgot to set domain ID.

Michal Privoznik (2):
  qemuDomainGetStats: Copy domain ID too
  virGetDomain: Set domain ID too

 daemon/remote.c |  5 +
 src/bhyve/bhyve_driver.c| 20 +-
 src/conf/domain_event.c | 11 +-
 src/conf/virdomainobjlist.c | 10 +++--
 src/datatypes.c |  8 ++--
 src/datatypes.h |  3 ++-
 src/esx/esx_driver.c| 49 ++---
 src/hyperv/hyperv_wmi.c | 15 +-
 src/libxl/libxl_driver.c| 20 +-
 src/libxl/libxl_migration.c |  2 +-
 src/lxc/lxc_driver.c| 20 +-
 src/openvz/openvz_driver.c  | 24 ++
 src/phyp/phyp_driver.c  | 12 +++
 src/qemu/qemu_driver.c  | 23 -
 src/qemu/qemu_migration.c   |  4 ++--
 src/remote/remote_driver.c  |  5 +
 src/test/test_driver.c  | 20 +-
 src/uml/uml_driver.c| 15 +-
 src/vbox/vbox_common.c  | 44 +++-
 src/vmware/vmware_driver.c  | 20 +-
 src/vz/vz_driver.c  | 22 ++--
 src/xen/xen_driver.c| 28 ++
 src/xenapi/xenapi_driver.c  | 30 +++
 23 files changed, 129 insertions(+), 281 deletions(-)

-- 
2.10.2

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

Re: [libvirt] [PATCH 4/4] news: Add template for a section

[Andrea Bolognani]

On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote:
> After the release it's necessary to add a new  section for the
> upcomming release. Add a template so that it does not have to be

s/upcomming/upcoming/

[...]
> @@ -20,6 +20,27 @@
>   Lines should be kept under 80 columns, and should not exceed 100 
>columns.
> 
>   This file is validated against docs/news.rng schema.
> +
> + Use the following template to add a new release section after the 
> release:

s/ after the release//


ACK with these two nits fixed.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 3/4] schema: Introduce schema for the news.xml file

[Andrea Bolognani]

On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote:
[...]
> @@ -18,6 +18,8 @@
>   each  tag is required to contain at least one  tag.
> 
>   Lines should be kept under 80 columns, and should not exceed 100 
>columns.
> +
> + This file is validated against docs/news.rng schema.

It's actually docs/schemas/news.rng ;)

[...]
> @@ -0,0 +1,73 @@
> +
> +http://relaxng.org/ns/structure/1.0"; 
> datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes";>

I would put datatypeLibrary= right under xmlns=, but it's
okay either way.

I'm no RNG expert, but the schema looks fine to me.

> @@ -237,6 +237,8 @@ mymain(void)
>  DO_TEST("storagevol.rng", "storagevolxml2xmlin", "storagevolxml2xmlout",
>  "storagevolschemadata");
> 
> +DO_TEST_FILE("news.rng", "../docs/news.xml");

You'll want to deal with how this interacts with VPATH
builds, as mentioned by Martin.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH v2] virNetDevIPCheckIPv6ForwardingCallback fixes

[Cédric Bosdonnat]

Add check for more than one RTA_OIF, even though this is rather
unlikely.

Get rid of the buggy switch / break as this code won't need to
handle more attributes.

Use VIR_WARNINGS_NO_CAST_ALIGN to fix impossible to fix
util/virnetdevip.c:560:17: error: cast increases required alignment of target 
type [-Werror=cast-align]
---
 Diff to v1:
   * Add error message
   * Use VIR_WARNINGS_NO_CAST_ALIGN
 src/util/virnetdevip.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index c9ac6baf7..726fa6c3e 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -556,15 +556,24 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct 
nlmsghdr *resp,
 if (resp->nlmsg_type != RTM_NEWROUTE)
 return ret;
 
-/* Extract a few attributes */
+/* Extract a device ID attribute */
+VIR_WARNINGS_NO_CAST_ALIGN
 for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
-switch (rta->rta_type) {
-case RTA_OIF:
+VIR_WARNINGS_RESET
+if (rta->rta_type == RTA_OIF) {
 oif = *(int *)RTA_DATA(rta);
 
+/* Should never happen: netlink message would be broken */
+if (ifname) {
+char *ifname2 = virNetDevGetName(oif);
+VIR_WARN("Single route has unexpected 2nd interface "
+ "- '%s' and '%s'", ifname, ifname2);
+VIR_FREE(ifname2);
+break;
+}
+
 if (!(ifname = virNetDevGetName(oif)))
 goto error;
-break;
 }
 }
 
-- 
2.12.0

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

Re: [libvirt] [PATCH 1/4] tests: schema: Add possibility to validate individual files

[Martin Kletzander]

On Tue, Mar 28, 2017 at 01:58:56PM +0200, Peter Krempa wrote:

Sometimes it may be desired to validate individual files against a
schema. Refactor the data structures to unify them and introduce a new
macro DO_TEST_FILE(schema, xmlfile) which will test the XML file against
the given schema file.
---
tests/virschematest.c | 34 +++---
1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/virschematest.c b/tests/virschematest.c
index faf66d642..beefabc96 100644
--- a/tests/virschematest.c
+++ b/tests/virschematest.c
@@ -35,6 +35,7 @@ VIR_LOG_INIT("tests.schematest");

struct testSchemaData {
virXMLValidatorPtr validator;
+const char *schema;
const char *xml_path;
};

@@ -140,15 +141,10 @@ testSchemaDirs(const char *schema, virXMLValidatorPtr 
validator, ...)
}


-struct testSchemaFileData {
-virXMLValidatorPtr validator;
-const char *schema;
-};
-
static int
testSchemaGrammar(const void *opaque)
{
-struct testSchemaFileData *data = (struct testSchemaFileData *) opaque;
+struct testSchemaData *data = (struct testSchemaData *) opaque;
char *schema_path;
int ret = -1;

@@ -171,7 +167,7 @@ static int
mymain(void)
{
int ret = 0;
-struct testSchemaFileData data;
+struct testSchemaData data;

memset(&data, 0, sizeof(data));

@@ -196,6 +192,30 @@ mymain(void)
}  \
} while (0)

+#define DO_TEST_FILE(sch, xmlfile) 
\
+do {   
\
+data.schema = sch; 
\
+data.xml_path = xmlfile;   
\
+if (virTestRun("test schema grammar file: " sch,   
\
+   testSchemaGrammar, &data) == 0) {   
\
+/* initialize the validator even if the schema test
\
+ * was skipped because of VIR_TEST_RANGE */
\
+if (!data.validator && testSchemaGrammar(&data) < 0) { 
\
+ret = -1;  
\
+break; 
\
+}  
\
+if (virTestRun("Checking " xmlfile " against " sch,
\
+   testSchemaFile, &data) < 0) 
\


testSchemaFile does not prepend abs_srcdir to the path (like
testSchemaDirs does), so after you add the test case, the check fails in
vpath builds (I just checked it).  I would suggest just a wrapper around
testSchemaFile that prepends that path or just use it when calling the
macro.


+ret = -1;  
\
+   
\
+virXMLValidatorFree(data.validator);   
\
+data.validator = NULL; 
\
+} else {   
\
+ret = -1;  
\
+}  
\
+} while (0)
+
+
DO_TEST("capability.rng", "capabilityschemadata", "xencapsdata");
DO_TEST("domain.rng", "domainschemadata", "qemuargv2xmldata",
"qemuxml2argvdata", "sexpr2xmldata", "xmconfigdata",
--
2.12.1

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


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

Re: [libvirt] [PATCH 8/8] docs: Improve documentation related to memory locking

[Luiz Capitulino]

On Mon, 27 Mar 2017 12:33:48 +0200
Andrea Bolognani  wrote:

> On Fri, 2017-03-24 at 13:58 -0400, Luiz Capitulino wrote:
> [...]
> > > +be allowed to swap them out, which might be required for some
> > > +workloads such as RT. For QEMU/KVM guests, the memory used by 
> > > the QEMU  
> > 
> > Minor, but I'd do s/RT/real-time. As this doc is for the general population,
> > RT may not be a know term for everyone.  
> 
> Sure.
> 
> > > +process itself will be locked too: unlike guest memory, this is 
> > > an
> > > +amount libvirt has no way of figuring out in advance, so it has 
> > > to
> > > +remove the limit on locked memory altogether. This can be very
> > > +dangerous as the host might run out of memory and be unable to 
> > > reclaim
> > > +it from the guest,  
> > 
> > I'd rewrite this to:
> > 
> > """
> > This option has a drawback and a possible security risk for the host. If
> > the host is running out of memory, it will be unable to reclaim the
> > memory locked by this guest which could cause the host to run out of
> > memory. In particular, a malicious guest could be able to lock as much
> > memory it wants, causing a DDoS attack in the host. For setups where
> > this may have a significant impact, it is highly recommended to use
> >  to prevent this attack.
> > """  
> 
> Another stab at it (which plugs into my original version):
> 
>   [...] remove the limit on locked memory altogether. Thus,
>   enabling this option opens up to a potential security risk:
>   the host will be unable to reclaim the locked memory back
>   from the guest when it's running out of memory, which means
>   a malicious guest allocating large amounts of locked memory
>   could cause a denial-of-service attach on the host. Because
>   of this, using the option is discouraged unless your [...]
> 
> Does it look reasonable?

That looks fine, although I'd drop "discouraged" because that's
not helpful to those who must use the feature. I think it's better
to objectively explain what the problems are and how to prevent or
mitigate them. That's what I tried to do in my paragraph.

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

Re: [libvirt] [PATCH 2/2] conf: do not steal pointers from the pool source

[Martin Kletzander]

On Tue, Mar 28, 2017 at 03:22:08PM +0200, Ján Tomko wrote:

Since commit fcbbb28 we steal the pointer to the storage pool
source name if there was no pool name specified.

Properly duplicate the string to avoid freeing it twice.

https://bugzilla.redhat.com/show_bug.cgi?id=1436400
---
src/conf/storage_conf.c |  5 +++--
tests/storagepoolxml2xmlout/pool-logical-noname.xml | 19 +++
tests/storagepoolxml2xmltest.c  |  1 +
3 files changed, 23 insertions(+), 2 deletions(-)
create mode 100644 tests/storagepoolxml2xmlout/pool-logical-noname.xml



The ACK from the previous one was meant for both patches, but I didn't
mention it for some reason.  So ACK to this one as well.


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

Re: [libvirt] [PATCH 2/4] news: Introduce rules for the schema file and fix offending lines

[Andrea Bolognani]

On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote:
> Add more strict rules for the news file and fix offending entries.

s/more strict/stricter/

> ---
>  docs/news.xml | 41 -
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 17186b954..024b659e8 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -5,9 +5,20 @@
>   This file will be processed to produce both HTML and plain text versions
>   of the release notes.
> 
> - Keep the style consistent with existing entries as much as possible:
> - each change should be documented by a short, one-sentence summary
> - and optionally a description where it's explained in more detail -->
> + Keep the style consistent with existing entries as much as possible.
> +
> + Each change should be documented by a short, one-sentence one-line 
> summary.
> + The summary should not contain any formatting tags.

"one-sentence one-line" is rather clunky, I would suggest
rewording it along the lines of

  Each change should be documented by a short, one-sentence
  summary, which should fit in a single line and should not
  contain any formatting tags.

> + Optionally use a description where the change can be explained in more
> + detail.

I think something along the lines of

  You can optionally add a description if you feel like
  the summary alone is not enough to document the change
  accurately.

would work better here.

> The description may contain a  tag for switching to
> + non-proportional font. No other tags are allowed.
> +
> + Each  tag is required to contain at least one  tag and
> + each  tag is required to contain at least one  tag.

I'm not sure the above two line are really needed. I would
omit them.


ACK if you take care of at least the commit message and the
first suggestion, I leave the remaining two up to you.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/2] schema: do not require name for certain pool types

[Martin Kletzander]

On Tue, Mar 28, 2017 at 03:22:07PM +0200, Ján Tomko wrote:

Pool types that have the VIR_STORAGE_POOL_SOURCE_NAME flag set
allow omitting the  element and instead fill out the pool name
from the  element.

Relax the schema to make  optional for these pool.


s/pool/pools/


Expressing that at least one of these is required is out of scope
of the schema.~


s/~$//

ACK and safe for freeze, obviously.


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

[libvirt] libvirt3.1.0 bug suspected: libvirtd restart, VM start/stop

[Stepan Andr]

Hi all,

It seems there is a bug for libvirtd which I couldn't find here

.

*Here is description:*
Version-Release number of selected component (if applicable):
Libvirt - 3.1.0 (built from tarball)

How reproducible:
100%

Precondition: libvirtd running. One VM is running, other(s) are 'shut off'.
Steps to Reproduce:
1. Stop libvirtd while VM#1 is running.
2. Start libvirtd (VM#1 is running).
3. Stop VM#1. Verify if VM#1 is present in the list of all VM's ('virsh
list --all').
4. Start VM#1. Verify if VM#1 is present in the list of all VM's ('virsh
list --all').
5. Go to step #3. Note: Steps may be needed to repeat two times.

Recover action:
 restart libvirtd service (service libvirtd restart/systemctl restart
libvirtd).

Actual results:
 #3. VM#1 disappears from the list of all VM's.

Expected results:
 #3. VM#1 should be present in the list of all VM's.

-- 
Best regards,
Stepan
# service libvirtd status
● libvirtd.service - Virtualization daemon
   Loaded: loaded (/usr/lib/systemd/system/libvirtd.service; disabled; vendor 
preset: enabled)
   Active: active (running) since Tue 2017-03-28 11:31:29 IST; 3s ago
 Docs: man:libvirtd(8)
   http://libvirt.org
 Main PID: 106843 (libvirtd)
   CGroup: /system.slice/libvirtd.service
   ├─106484 qemu-system-x86_64 -enable-kvm -name 
guest=ubuntu1604_01,debug-threads=on -S -machine 
pc-i440fx-2.5,accel=kvm,usb=off,dump-guest-core=off -cpu host -m 2048 -realtime 
mlock=
   ├─106843 /usr/sbin/libvirtd
   ├─194319 /usr/sbin/dnsmasq 
--conf-file=/var/lib/libvirt/dnsmasq/default.conf --leasefile-ro 
--dhcp-script=/usr/libexec/libvirt_leaseshelper
   └─194320 /usr/sbin/dnsmasq 
--conf-file=/var/lib/libvirt/dnsmasq/default.conf --leasefile-ro 
--dhcp-script=/usr/libexec/libvirt_leaseshelper

Mar 28 11:31:29 ubuntu1604 libvirtd[106843]: 2017-03-28 10:31:29.406+: 
106859: info : libvirt version: 3.1.0

SNIP

Mar 28 11:31:29 ubuntu1604 libvirtd[106843]: 2017-03-28 10:31:29.406+: 
106859: error : virNetworkAssignDefLocked:568 : operation failed: network 
'default' already exists with uuid 81b506
Mar 28 11:31:29 ubuntu1604 dnsmasq[194319]: read /etc/hosts - 7 addresses
Mar 28 11:31:29 ubuntu1604 dnsmasq[194319]: read 
/var/lib/libvirt/dnsmasq/default.addnhosts - 0 addresses
Mar 28 11:31:29 ubuntu1604 dnsmasq-dhcp[194319]: read 
/var/lib/libvirt/dnsmasq/default.hostsfile
Mar 28 11:31:31 ubuntu1604 libvirtd[106843]: 2017-03-28 10:31:31.127+: 
106859: error : virDomainObjListAddLocked:289 : operation failed: domain 
'ubuntu1604_03' already exists with uui
Mar 28 11:31:31 ubuntu1604 libvirtd[106843]: 2017-03-28 10:31:31.128+: 
106859: error : virDomainObjListAddLocked:289 : operation failed: domain 
'ubuntu1604_01' already exists with uui
Mar 28 11:31:31 ubuntu1604 libvirtd[106843]: 2017-03-28 10:31:31.135+: 
106859: error : virDomainObjListAddLocked:289 : operation failed: domain 
'ubuntu1604_01' already exists with uui
Mar 28 11:31:31 ubuntu1604 libvirtd[106843]: 2017-03-28 10:31:31.136+: 
106859: error : qemuDomainDefPostParse:2654 : internal error: missing machine 
type
# virsh list --all
 IdName   State

 2 ubuntu1604_01   running
 - ubuntu1604_02   shut off
 - ubuntu1604_03   shut off

# virsh destroy ubuntu1604_01
Domain ubuntu1604_01 destroyed

# virsh start ubuntu1604_01
error: failed to get domain 'ubuntu1604_01'
error: Domain not found: no domain with matching name 'ubuntu1604_01'

# virsh list --all
 IdName   State

 - ubuntu1604_02   shut off
 - ubuntu1604_03   shut off

# service libvirtd restart
# virsh list --all
 IdName   State

 - ubuntu1604_01   shut off
 - ubuntu1604_02   shut off
 - ubuntu1604_03   shut off

# virsh --version
3.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/8] Revert "qemu: Forbid without "

[Luiz Capitulino]

On Mon, 27 Mar 2017 10:33:52 +0200
Andrea Bolognani  wrote:

> On Fri, 2017-03-24 at 13:36 -0400, Luiz Capitulino wrote:
> > > Turns out this check is excessively strict: there are ways
> > > other than  to raise the memory locking
> > > limit for QEMU processes, one prominent example being
> > > tweaking /etc/security/limits.conf.  
> > 
> > Actually, it seems that limits.conf doesn't work with libvirt
> > as mentioned by Daniel in another thread. I didn't know this
> > myself btw.
> > 
> > This makes this series even more important because only through
> > libvirt we can set this limit to infinity.  
> 
> Well, it *does* work if you set it up properly, eg. raise the
> memory locking limit for the user under which libvirtd will
> run instead of the user under which QEMU processes will run.

Doesn't libvirtd run as root?

> 
> Doing so is very counter-intuitive, though.
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 


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

[libvirt] [PATCH 1/2] schema: do not require name for certain pool types

[Ján Tomko]

Pool types that have the VIR_STORAGE_POOL_SOURCE_NAME flag set
allow omitting the  element and instead fill out the pool name
from the  element.

Relax the schema to make  optional for these pool.
Expressing that at least one of these is required is out of scope
of the schema.~
---
 docs/schemas/storagepool.rng   | 27 +-
 tests/storagepoolxml2xmlin/pool-logical-noname.xml | 18 +++
 2 files changed, 39 insertions(+), 6 deletions(-)
 create mode 100644 tests/storagepoolxml2xmlin/pool-logical-noname.xml

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 6219ce5..8386f29 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -70,7 +70,7 @@
   logical
 
 
-  
+  
   
   
   
@@ -132,7 +132,7 @@
   rbd
 
 
-  
+  
   
   
 
@@ -143,7 +143,7 @@
   sheepdog
 
 
-  
+  
   
   
 
@@ -154,7 +154,7 @@
   gluster
 
 
-  
+  
   
   
 
@@ -165,7 +165,7 @@
   zfs
 
 
-  
+  
   
   
   
@@ -179,7 +179,7 @@
   vstorage
 
 
-  
+  
   
   
   
@@ -205,6 +205,21 @@
 
   
 
+  
+
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
   
 
   
diff --git a/tests/storagepoolxml2xmlin/pool-logical-noname.xml 
b/tests/storagepoolxml2xmlin/pool-logical-noname.xml
new file mode 100644
index 000..ad3f88d
--- /dev/null
+++ b/tests/storagepoolxml2xmlin/pool-logical-noname.xml
@@ -0,0 +1,18 @@
+
+  1c13165a-d0f4-3aee-b447-30fb38789091
+  99891544064
+  99220455424
+  671088640
+  
+zily
+
+  
+  
+/dev/zily
+
+  0700
+  0
+  0
+
+  
+
-- 
2.10.2

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

Re: [libvirt] [PATCH 1/4] tests: schema: Add possibility to validate individual files

[Andrea Bolognani]

On Tue, 2017-03-28 at 13:58 +0200, Peter Krempa wrote:
[...]
> @@ -196,6 +192,30 @@ mymain(void)
>  }
>  \
>  } while (0)
> 
> +#define DO_TEST_FILE(sch, xmlfile)   
>   \
> +do { 
>   \
> +data.schema = sch;   
>   \
> +data.xml_path = xmlfile; 
>   \
> +if (virTestRun("test schema grammar file: " sch, 
>   \
> +   testSchemaGrammar, &data) == 0) { 
>   \
> +/* initialize the validator even if the schema test  
>   \
> + * was skipped because of VIR_TEST_RANGE */  
>   \
> +if (!data.validator && testSchemaGrammar(&data) < 0) {   
>   \
> +ret = -1;
>   \
> +break;   
>   \
> +}
>   \
> +if (virTestRun("Checking " xmlfile " against " sch,  
>   \
> +   testSchemaFile, &data) < 0)   
>   \
> +ret = -1;
>   \
> + 
>   \
> +virXMLValidatorFree(data.validator); 
>   \
> +data.validator = NULL;   
>   \
> +} else { 
>   \
> +ret = -1;
>   \
> +}
>   \
> +} while (0)
> +
> +

Only one empty line here, please.


Now that you've introduced DO_TEST_FILE(), I think it would
make sense to rename DO_TEST() to DO_TEST_DIRS() for clarity.

ACK whether you feel the same or not.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH 2/2] conf: do not steal pointers from the pool source

[Ján Tomko]

Since commit fcbbb28 we steal the pointer to the storage pool
source name if there was no pool name specified.

Properly duplicate the string to avoid freeing it twice.

https://bugzilla.redhat.com/show_bug.cgi?id=1436400
---
 src/conf/storage_conf.c |  5 +++--
 tests/storagepoolxml2xmlout/pool-logical-noname.xml | 19 +++
 tests/storagepoolxml2xmltest.c  |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)
 create mode 100644 tests/storagepoolxml2xmlout/pool-logical-noname.xml

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 585ca71..fe0f0bc 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -710,8 +710,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 
 ret->name = virXPathString("string(./name)", ctxt);
 if (ret->name == NULL &&
-options->flags & VIR_STORAGE_POOL_SOURCE_NAME)
-ret->name = ret->source.name;
+options->flags & VIR_STORAGE_POOL_SOURCE_NAME &&
+VIR_STRDUP(ret->name, ret->source.name) < 0)
+goto error;
 if (ret->name == NULL) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("missing pool source name element"));
diff --git a/tests/storagepoolxml2xmlout/pool-logical-noname.xml 
b/tests/storagepoolxml2xmlout/pool-logical-noname.xml
new file mode 100644
index 000..a5e0ead
--- /dev/null
+++ b/tests/storagepoolxml2xmlout/pool-logical-noname.xml
@@ -0,0 +1,19 @@
+
+  zily
+  1c13165a-d0f4-3aee-b447-30fb38789091
+  0
+  0
+  0
+  
+zily
+
+  
+  
+/dev/zily
+
+  0700
+  0
+  0
+
+  
+
diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
index 79bdc26..355871c 100644
--- a/tests/storagepoolxml2xmltest.c
+++ b/tests/storagepoolxml2xmltest.c
@@ -79,6 +79,7 @@ mymain(void)
 DO_TEST("pool-logical");
 DO_TEST("pool-logical-nopath");
 DO_TEST("pool-logical-create");
+DO_TEST("pool-logical-noname");
 DO_TEST("pool-disk");
 DO_TEST("pool-disk-device-nopartsep");
 DO_TEST("pool-iscsi");
-- 
2.10.2

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

[libvirt] [PATCH 0/2] conf: do not steal pointers from the pool source

[Ján Tomko]

https://bugzilla.redhat.com/show_bug.cgi?id=1436400

Ján Tomko (2):
  schema: do not require name for certain pool types
  conf: do not steal pointers from the pool source

 docs/schemas/storagepool.rng   | 27 +-
 src/conf/storage_conf.c|  5 ++--
 tests/storagepoolxml2xmlin/pool-logical-noname.xml | 18 +++
 .../storagepoolxml2xmlout/pool-logical-noname.xml  | 19 +++
 tests/storagepoolxml2xmltest.c |  1 +
 5 files changed, 62 insertions(+), 8 deletions(-)
 create mode 100644 tests/storagepoolxml2xmlin/pool-logical-noname.xml
 create mode 100644 tests/storagepoolxml2xmlout/pool-logical-noname.xml

-- 
2.10.2

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

[libvirt] [PATCH 0/4] news: Add schema validation and tweak few details

[Peter Krempa]

The file caused quite some trouble. Add schema validation so that breakage
is kept to a minimum.

Peter Krempa (4):
  tests: schema: Add possibility to validate individual files
  news: Introduce rules for the schema file and fix offending lines
  schema: Introduce schema for the news.xml file
  news: Add template for a  section

 docs/news.xml | 64 +++-
 docs/schemas/news.rng | 73 +++
 tests/virschematest.c | 36 -
 3 files changed, 153 insertions(+), 20 deletions(-)
 create mode 100644 docs/schemas/news.rng

-- 
2.12.1

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

[libvirt] [PATCH 2/4] news: Introduce rules for the schema file and fix offending lines

[Peter Krempa]

Add more strict rules for the news file and fix offending entries.
---
 docs/news.xml | 41 -
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 17186b954..024b659e8 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -5,9 +5,20 @@
  This file will be processed to produce both HTML and plain text versions
  of the release notes.

- Keep the style consistent with existing entries as much as possible:
- each change should be documented by a short, one-sentence summary
- and optionally a description where it's explained in more detail -->
+ Keep the style consistent with existing entries as much as possible.
+
+ Each change should be documented by a short, one-sentence one-line 
summary.
+ The summary should not contain any formatting tags.
+
+ Optionally use a description where the change can be explained in more
+ detail. The description may contain a  tag for switching to
+ non-proportional font. No other tags are allowed.
+
+ Each  tag is required to contain at least one  tag and
+ each  tag is required to contain at least one  tag.
+
+ Lines should be kept under 80 columns, and should not exceed 100 columns.
+ -->

 
   
@@ -355,7 +366,7 @@
   
   
 
-  nss: Introduce libvirt_guest
+  nss: Introduce libvirt_guest
 
 
   New libvirt_guest nss module that translates libvirt
@@ -615,7 +626,7 @@
 
   The new libssh transport allows one to connect to a running
   libvirtd via SSH, using the libssh library; for example:
-  qemu+libssh://server/system.
+  qemu+libssh://server/system.
 
   
   
@@ -629,17 +640,22 @@
   
   
 
-  qemu: Users can now enable debug logging for native gluster
-  volumes in qemu using the "gluster_debug_level" option in qemu.conf
+  Allow debugging of gluster volumes in qemu
 
+
+  Users can now enable debug logging for native gluster
+  volumes in qemu using the "gluster_debug_level" option in qemu.conf
+
   
   
 
-  memory hotplug: Slot numbers for memory devices are now
-  automatically allocated and thus persistent. In addition slot numbers
-  can be specified without providing a base address, which simplifies
-  user configuration
+  Pre-allocate memory slots for memory hotplug
 
+
+  Slot numbers for memory devices are now automatically allocated and
+  thus persistent. In addition slot numbers can be specified without
+  providing a base address, which simplifies user configuration
+
   
   
 
@@ -669,8 +685,7 @@
   
   
 
-  virsh: Add support for passing an alternative persistent XML
-  to migrate command
+  virsh: Add support for passing an alternative persistent XML to 
migrate command
 
   
   
-- 
2.12.1

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

[libvirt] [PATCH 3/4] schema: Introduce schema for the news.xml file

[Peter Krempa]

Since this file gets changed (and broken) rather often, introduce a
schema file so that the test suite can validate it.
---
 docs/news.xml |  2 ++
 docs/schemas/news.rng | 73 +++
 tests/virschematest.c |  2 ++
 3 files changed, 77 insertions(+)
 create mode 100644 docs/schemas/news.rng

diff --git a/docs/news.xml b/docs/news.xml
index 024b659e8..c2a2917c1 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -18,6 +18,8 @@
  each  tag is required to contain at least one  tag.

  Lines should be kept under 80 columns, and should not exceed 100 columns.
+
+ This file is validated against docs/news.rng schema.
  -->

 
diff --git a/docs/schemas/news.rng b/docs/schemas/news.rng
new file mode 100644
index 0..94a6870c1
--- /dev/null
+++ b/docs/schemas/news.rng
@@ -0,0 +1,73 @@
+
+http://relaxng.org/ns/structure/1.0"; 
datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes";>
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+
+  v[0-9]+\.[0-9]+\.[0-9]+
+
+  
+  
+
+  [0-9]{4}-[0-9]{2}-[0-9]{2}|unreleased
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+\n[^\n]+\n +
+  
+  
+
+  
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+  
+
+
diff --git a/tests/virschematest.c b/tests/virschematest.c
index beefabc96..e0e5872db 100644
--- a/tests/virschematest.c
+++ b/tests/virschematest.c
@@ -237,6 +237,8 @@ mymain(void)
 DO_TEST("storagevol.rng", "storagevolxml2xmlin", "storagevolxml2xmlout",
 "storagevolschemadata");

+DO_TEST_FILE("news.rng", "../docs/news.xml");
+
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }

-- 
2.12.1

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

[libvirt] [PATCH 4/4] news: Add template for a section

[Peter Krempa]

After the release it's necessary to add a new  section for the
upcomming release. Add a template so that it does not have to be
compiled over and over again.
---
 docs/news.xml | 21 +
 1 file changed, 21 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index c2a2917c1..1ebe0eec3 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -20,6 +20,27 @@
  Lines should be kept under 80 columns, and should not exceed 100 columns.

  This file is validated against docs/news.rng schema.
+
+ Use the following template to add a new release section after the release:
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+
+  
+
+  
+
+  
+
  -->

 
-- 
2.12.1

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

[libvirt] [PATCH 1/4] tests: schema: Add possibility to validate individual files

[Peter Krempa]

Sometimes it may be desired to validate individual files against a
schema. Refactor the data structures to unify them and introduce a new
macro DO_TEST_FILE(schema, xmlfile) which will test the XML file against
the given schema file.
---
 tests/virschematest.c | 34 +++---
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/virschematest.c b/tests/virschematest.c
index faf66d642..beefabc96 100644
--- a/tests/virschematest.c
+++ b/tests/virschematest.c
@@ -35,6 +35,7 @@ VIR_LOG_INIT("tests.schematest");

 struct testSchemaData {
 virXMLValidatorPtr validator;
+const char *schema;
 const char *xml_path;
 };

@@ -140,15 +141,10 @@ testSchemaDirs(const char *schema, virXMLValidatorPtr 
validator, ...)
 }


-struct testSchemaFileData {
-virXMLValidatorPtr validator;
-const char *schema;
-};
-
 static int
 testSchemaGrammar(const void *opaque)
 {
-struct testSchemaFileData *data = (struct testSchemaFileData *) opaque;
+struct testSchemaData *data = (struct testSchemaData *) opaque;
 char *schema_path;
 int ret = -1;

@@ -171,7 +167,7 @@ static int
 mymain(void)
 {
 int ret = 0;
-struct testSchemaFileData data;
+struct testSchemaData data;

 memset(&data, 0, sizeof(data));

@@ -196,6 +192,30 @@ mymain(void)
 }  
\
 } while (0)

+#define DO_TEST_FILE(sch, xmlfile) 
\
+do {   
\
+data.schema = sch; 
\
+data.xml_path = xmlfile;   
\
+if (virTestRun("test schema grammar file: " sch,   
\
+   testSchemaGrammar, &data) == 0) {   
\
+/* initialize the validator even if the schema test
\
+ * was skipped because of VIR_TEST_RANGE */
\
+if (!data.validator && testSchemaGrammar(&data) < 0) { 
\
+ret = -1;  
\
+break; 
\
+}  
\
+if (virTestRun("Checking " xmlfile " against " sch,
\
+   testSchemaFile, &data) < 0) 
\
+ret = -1;  
\
+   
\
+virXMLValidatorFree(data.validator);   
\
+data.validator = NULL; 
\
+} else {   
\
+ret = -1;  
\
+}  
\
+} while (0)
+
+
 DO_TEST("capability.rng", "capabilityschemadata", "xencapsdata");
 DO_TEST("domain.rng", "domainschemadata", "qemuargv2xmldata",
 "qemuxml2argvdata", "sexpr2xmldata", "xmconfigdata",
-- 
2.12.1

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

Re: [libvirt] [PATCH] storage: Fix build on i686

[Michal Privoznik]

On 03/28/2017 12:55 PM, Jiri Denemark wrote:

off_t is signed and it's size is the same as long only on 64b archs.
Thus it cannot be formatted as %lu.

Signed-off-by: Jiri Denemark 
---
 src/storage/storage_backend_logical.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index 9ca6fd43b..d87aaf0b6 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -116,9 +116,9 @@ virStorageBackendLogicalInitializeDevice(const char *path)

 if (size < 4 * PV_BLANK_SECTOR_SIZE) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("cannot initialize '%s' detected size='%lu' less "
+   _("cannot initialize '%s' detected size='%zd' less "
  "than minimum required='%d"),
- path, size, 4 * PV_BLANK_SECTOR_SIZE);
+ path, (ssize_t) size, 4 * PV_BLANK_SECTOR_SIZE);
 goto cleanup;
 }
 if ((size = lseek(fd, 0, SEEK_SET)) == (off_t)-1) {



ACK

Michal

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

[libvirt] [PATCH] storage: Fix build on i686

[Jiri Denemark]

off_t is signed and it's size is the same as long only on 64b archs.
Thus it cannot be formatted as %lu.

Signed-off-by: Jiri Denemark 
---
 src/storage/storage_backend_logical.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index 9ca6fd43b..d87aaf0b6 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -116,9 +116,9 @@ virStorageBackendLogicalInitializeDevice(const char *path)
 
 if (size < 4 * PV_BLANK_SECTOR_SIZE) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("cannot initialize '%s' detected size='%lu' less "
+   _("cannot initialize '%s' detected size='%zd' less "
  "than minimum required='%d"),
- path, size, 4 * PV_BLANK_SECTOR_SIZE);
+ path, (ssize_t) size, 4 * PV_BLANK_SECTOR_SIZE);
 goto cleanup;
 }
 if ((size = lseek(fd, 0, SEEK_SET)) == (off_t)-1) {
-- 
2.12.2

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

Re: [libvirt] [PATCHv2 0/2] Restore logical pool functionality

[John Ferlan]

On 03/28/2017 04:40 AM, Ján Tomko wrote:
> John Ferlan (1):
>   storage: Better describe logical pool creation/definition parameters
> 
> Ján Tomko (1):
>   Revert "storage: Better describe logical pool creation/definition
> parameters"
> 
>  src/conf/storage_conf.c | 8 
>  tools/virsh.pod | 7 +++
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 

ACK series,

John


Since it was asked... The answer to "why" comes from reading the bz and
considering the frustration of someone that was trying to use the virsh
pool-define-as command to create a logical pool and well assuming more
about the name given the test pools I have. In the long run source.name
is the key for the pool and volume group connection. There there was
code that copied source.name from name when source.name was not
provided, so something just clicked that said well the opposite must be
true to if someone provides both then they should be the same.

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

Re: [libvirt] [PATCH] news: Fix typo in element name

[Martin Kletzander]

On Tue, Mar 28, 2017 at 10:13:29AM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Oops, pushed as trivial. We really need a schema for news.xml...



Patches are welcome ;-)


docs/news.xml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 85a1d4d97..17186b954 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -72,7 +72,7 @@
  introducing a new host device type in the XML.

  
-  
+  

  qemu: Add support for setting TSC frequency

@@ -80,7 +80,7 @@
  Setting TSC frequency is required to enable migration for domains
  with 'invtsc' CPU feature turned on.

-  
+  


  
--
2.12.2

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


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

Re: [libvirt] [PATCH v2 0/7] Memory locking fixes

[Andrea Bolognani]

On Mon, 2017-03-27 at 22:16 +0200, Martin Kletzander wrote:
> About that ^^ there's still something to fix, but if you follow my
> advice, ACK series.

Thanks. Pushed on account of the fact that it was ACKed before
the freeze started.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 1/2] networkUpdateState: Create virMacMap module more frequently

[Michal Privoznik]

On 03/28/2017 02:22 AM, Laine Stump wrote:

On 03/22/2017 10:43 AM, Michal Privoznik wrote:






Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a753cd78b..0ea8e2348 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj,
 {
 virNetworkDriverStatePtr driver = opaque;
 dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver);
+char *macMapFile = NULL;
 int ret = -1;

 virObjectLock(obj);
@@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj,
 /* If bridge doesn't exist, then mark it inactive */
 if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1))
 obj->active = 0;
+
+if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge)))
+goto cleanup;
+
+if (!(obj->macmap = virMacMapNew(macMapFile)))
+goto cleanup;
+
 break;



... but from what I can understand, the only differences are:

1) the macMapFile used to be regenerated right after reading the
radvdpidfile (which in most cases doesn't exist because I think most of
the time that same duty is handled by dnsmasq instead), and now it is
regenerated *just before* reading radvdpidfile.


I don't think this is any problem. It's just an ordering issue. Those 
two features are orthogonal.




2) it used to be regenerated for any network with a def->bridge (so it
would also happen for "unmanaged" bridge networks, where libvirt just
points to an already-existing bridge), and it is now regenerated only
for networks where libvirt creates and destroys the bridge (and might
have a dnsmasq instance running.


Ah. Is that right? I agree that the code is a bit unclear, but the 
following should be true:


1) when a network is being freshly started up, the virMacMap module is 
created and stored in the network object if and only if dnsmasq is 
spawned (because only then it's us who assigns IP addresses).


2) when an interface is allocated/freed from a network that has the 
module, the $br.macs file is updated accordingly. The file is created on 
the first interface allocation, and unlinked on network undef.


3) when the daemon restarts, networkUpdateState() is called for every 
running network. And if the $br.macs file exists for given network, the 
module is created (we are reconstructing the network objects after all) 
and the file is parsed to restore the previous state. Note, that the 
dnsmasq is not spawned again - it kept running while libvirtd was 
restarting.


Now, there are two problems that I see:

a) if there is no interface added in the 2nd step and the libvirt daemon 
is restarted, in the 3rd step the file does not exist and thus the code 
thinks no virMacMap module was used for the network so it does not 
create one. This is obviously a bug.


b) if you want to have the module for your network, you need to shut it 
down (and thus cut off your domains from the connectivity) and start 
over again. It's very annoying. When implementing this, my reasoning was 
that it's better to give no results than partial results, it's better to 
not have no $br.macs file than have a file containing just newly 
allocated interfaces. Well, I was wrong. I think people don't want to 
destroy their network unless really necessary. More so if the network 
continues running even when the daemon is killed.


Therefore, what I am suggesting here is to rework step 3) so that the 
module is created more frequently. It would solve both problems I'm 
describing.


Michal

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

[libvirt] [PATCHv2 1/2] Revert "storage: Better describe logical pool creation/definition parameters"

[Ján Tomko]

This reverts commit ca4515d2639057020c749470f390fe1f5981e91e
which also included a functional change that broke logical storage pools
not named after their volume groups.
---
 src/conf/storage_conf.c | 8 
 tools/virsh.pod | 7 ---
 2 files changed, 15 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5213503..585ca71 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -760,14 +760,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (VIR_STRDUP(ret->source.name, ret->name) < 0)
 goto error;
 }
-if (ret->type == VIR_STORAGE_POOL_LOGICAL &&
-STRNEQ(ret->name, ret->source.name)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("for a logical pool, the pool name='%s' "
- "must match the pool source name='%s'"),
-   ret->name, ret->source.name);
-goto error;
-}
 }
 
 if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) &&
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 55b71a9..43124ba 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3639,13 +3639,6 @@ follow-up command to build the pool. The I<--overwrite> 
and
 I<--no-overwrite> flags follow the same rules as B. If
 just I<--build> is provided, then B is called with no flags.
 
-For a "logical" pool only [I<--name>] needs to be provided. The [I<--name>]
-must match the Volume Group name for which the pool is being defined or
-created. The [I<--source-name>] if provided must match the Volume Group
-name. If not provided, one will be generated using the [I<--name>]. If
-provided the [I<--target>] is ignored and a target source is generated
-using the [I<--source-name>] (or as generated from the [I<--name>]).
-
 =item B I
 
 Define an inactive persistent storage pool or modify an existing persistent one
-- 
2.10.2

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

Re: [libvirt] [PATCH v2 6/7] tests: Introduce QEMU memory locking limit tests

[Andrea Bolognani]

On Mon, 2017-03-27 at 22:15 +0200, Martin Kletzander wrote:
[...]
> Looking at this function I don't think we really understood each other.
> Looks like I explain better with code than words, so let me fix that
> right up.  ACK with the following squashed in:

I understood your suggestion, I just did not realize the
full extent to which I could get rid of superflous code ;)

Once again, following your advice resulted in a much leaner
and readable test case, which is great! Thanks a bunch :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCHv2 0/2] Restore logical pool functionality

[Ján Tomko]

John Ferlan (1):
  storage: Better describe logical pool creation/definition parameters

Ján Tomko (1):
  Revert "storage: Better describe logical pool creation/definition
parameters"

 src/conf/storage_conf.c | 8 
 tools/virsh.pod | 7 +++
 2 files changed, 3 insertions(+), 12 deletions(-)

-- 
2.10.2

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

[libvirt] [PATCHv2 2/2] storage: Better describe logical pool creation/definition parameters

[Ján Tomko]

From: John Ferlan 

https://bugzilla.redhat.com/show_bug.cgi?id=1398087

Clean up the virsh man page description for --pool-create-as in order
to better describe how the various arguments are used when creating
(or defining) a logical pool.

Signed-off-by: Ján Tomko 
---
 tools/virsh.pod | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 43124ba..9a52d67 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3639,6 +3639,12 @@ follow-up command to build the pool. The I<--overwrite> 
and
 I<--no-overwrite> flags follow the same rules as B. If
 just I<--build> is provided, then B is called with no flags.
 
+For a "logical" pool only [I<--name>] needs to be provided. The
+[I<--source-name>] if provided must match the Volume Group name.
+If not provided, one will be generated using the [I<--name>]. If
+provided the [I<--target>] is ignored and a target source is generated
+using the [I<--source-name>] (or as generated from the [I<--name>]).
+
 =item B I
 
 Define an inactive persistent storage pool or modify an existing persistent one
-- 
2.10.2

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

Re: [libvirt] [PATCH] Revert "storage: Better describe logical pool creation/definition parameters"

[Pavel Hrdina]

On Tue, Mar 28, 2017 at 09:51:22AM +0200, Ján Tomko wrote:
> This reverts commit ca4515d2639057020c749470f390fe1f5981e91e
> which broke the functionality of logical storage pools
> not named after their volume groups.

I think that we should also revert commit 5edf9aaf54 because the news
change basically describes what this patch reverts.

ACK

Pavel


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

[libvirt] [PATCH] news: Fix typo in element name

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---

Oops, pushed as trivial. We really need a schema for news.xml...

 docs/news.xml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 85a1d4d97..17186b954 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -72,7 +72,7 @@
   introducing a new host device type in the XML.
 
   
-  
+  
 
   qemu: Add support for setting TSC frequency
 
@@ -80,7 +80,7 @@
   Setting TSC frequency is required to enable migration for domains
   with 'invtsc' CPU feature turned on.
 
-  
+  
 
 
   
-- 
2.12.2

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

[libvirt] [PATCH v2] qemu: command: align disk serial check to schema

[Nikolay Shirokovskiy]

Disk serial schema has extra '.+' allowed characters in comparison
with check in code. Looks like there is no reason for that as qemu
allows any character AFAIK for serial. This discrepancy is originated
in 85d15b51 where ability to add serial was added.
---

Diff from v1:

* fix xml2argv disk serial test to use all valid chars

Looks like there is no existing infrastructure to test every invalid character.

 src/qemu/qemu_command.c  | 2 +-
 tests/qemuxml2argvdata/qemuxml2argv-disk-serial.args | 2 +-
 tests/qemuxml2argvdata/qemuxml2argv-disk-serial.xml  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c76f923..c5369b0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -432,7 +432,7 @@ qemuBuildIoEventFdStr(virBufferPtr buf,
 }
 
 #define QEMU_SERIAL_PARAM_ACCEPTED_CHARS \
-  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_ "
+  "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_ .+"
 
 static int
 qemuSafeSerialParamValue(const char *value)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.args 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.args
index 2cefdca..fa0fc93 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.args
@@ -18,6 +18,6 @@ QEMU_AUDIO_DRV=none \
 -boot c \
 -usb \
 -drive 'file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-1,\
-serial=\ \ WD-WMAP9A966149' \
+serial=abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_\ .+' \
 -device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.xml
index 565462e..d54d73b 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-serial.xml
@@ -17,7 +17,7 @@
 
   
   
-WD-WMAP9A966149
+  abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_ 
.+
   
 
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH] Revert "storage: Better describe logical pool creation/definition parameters"

[Peter Krempa]

On Tue, Mar 28, 2017 at 09:51:22 +0200, Ján Tomko wrote:
> This reverts commit ca4515d2639057020c749470f390fe1f5981e91e
> which broke the functionality of logical storage pools
> not named after their volume groups.
> ---
>  src/conf/storage_conf.c | 8 
>  tools/virsh.pod | 7 ---
>  2 files changed, 15 deletions(-)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 5213503..585ca71 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -760,14 +760,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>  if (VIR_STRDUP(ret->source.name, ret->name) < 0)
>  goto error;
>  }
> -if (ret->type == VIR_STORAGE_POOL_LOGICAL &&
> -STRNEQ(ret->name, ret->source.name)) {
> -virReportError(VIR_ERR_XML_ERROR,
> -   _("for a logical pool, the pool name='%s' "
> - "must match the pool source name='%s'"),
> -   ret->name, ret->source.name);
> -goto error;
> -}

This check does not make much sense.

>  }
>  
>  if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) &&
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 55b71a9..43124ba 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -3639,13 +3639,6 @@ follow-up command to build the pool. The 
> I<--overwrite> and
>  I<--no-overwrite> flags follow the same rules as B. If
>  just I<--build> is provided, then B is called with no flags.
>  
> -For a "logical" pool only [I<--name>] needs to be provided. The [I<--name>]

The first sentence might make sense, since you don't change the code
that would invalidate it.

> -must match the Volume Group name for which the pool is being defined or
> -created. The [I<--source-name>] if provided must match the Volume Group

This needs to go.

> -name. If not provided, one will be generated using the [I<--name>]. If
> -provided the [I<--target>] is ignored and a target source is generated
> -using the [I<--source-name>] (or as generated from the [I<--name>]).

This might make sense though.

> -

ACK if you don't completely revert the documentation change.


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

Re: [libvirt] [PATCH] hostdev: Fix build with GCC's static analysis in mdev

[Martin Kletzander]

On Mon, Mar 27, 2017 at 08:24:58PM +0200, Martin Kletzander wrote:

On Mon, Mar 27, 2017 at 12:30:23PM -0400, Laine Stump wrote:

On 03/27/2017 11:40 AM, John Ferlan wrote:



On 03/27/2017 11:30 AM, Martin Kletzander wrote:

Similarly to eec3b255d26e7b38bdb0830990569fd91aee661f, fix build with
lv_cv_static_analysis=yes.

Caused by a4a39d90ab4930750bcbcfccffdf6bb6d310b5d5
---
 src/util/virhostdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)



Sure that's another way to fix it... Could have also gone with the
removal of NONNULL in the prototype like I did.  IDC whichever way is
deemed "more appropriate"...

Seeing as drv_name is only ever passed as QEMU_DRIVER_NAME, but dom_name
is passed from 'name' which doesn't have the NONNULL on it, that's why I
chose removing NONNULL from the prototype.


I think I prefer John's way too. (At least partly because I dislike
ATTRIBUTE_NONNULL() and would like to see as many of them as possible go
away).



Actually, I do too.  I would love to remove all of them.  But for that
we'd have to reach a decision.  However there are not only cons, some
pros can be that the compiler is able to make better guesses or not have
to be guessing at all.  As a counter-argument, there are way more
attribute we could use and we don't, plus libvirt is not the CPU hogger
where we would have to go optimize so low.  So while the answer is not
clear, I wouldn't, as mentioned before, be against removing all such
occurrences, maybe except public APIs, if someone happens to have a good
reason for that.


I see your commit from 2012 now.  And I think we might consider removing
attribute_nonnull altogether.  Let me propose a patch...


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


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

[libvirt] [PATCH] Revert "storage: Better describe logical pool creation/definition parameters"

[Ján Tomko]

This reverts commit ca4515d2639057020c749470f390fe1f5981e91e
which broke the functionality of logical storage pools
not named after their volume groups.
---
 src/conf/storage_conf.c | 8 
 tools/virsh.pod | 7 ---
 2 files changed, 15 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5213503..585ca71 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -760,14 +760,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (VIR_STRDUP(ret->source.name, ret->name) < 0)
 goto error;
 }
-if (ret->type == VIR_STORAGE_POOL_LOGICAL &&
-STRNEQ(ret->name, ret->source.name)) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("for a logical pool, the pool name='%s' "
- "must match the pool source name='%s'"),
-   ret->name, ret->source.name);
-goto error;
-}
 }
 
 if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) &&
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 55b71a9..43124ba 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3639,13 +3639,6 @@ follow-up command to build the pool. The I<--overwrite> 
and
 I<--no-overwrite> flags follow the same rules as B. If
 just I<--build> is provided, then B is called with no flags.
 
-For a "logical" pool only [I<--name>] needs to be provided. The [I<--name>]
-must match the Volume Group name for which the pool is being defined or
-created. The [I<--source-name>] if provided must match the Volume Group
-name. If not provided, one will be generated using the [I<--name>]. If
-provided the [I<--target>] is ignored and a target source is generated
-using the [I<--source-name>] (or as generated from the [I<--name>]).
-
 =item B I
 
 Define an inactive persistent storage pool or modify an existing persistent one
-- 
2.10.2

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

Re: [libvirt] [PATCH v2 3/4] storage: Better describe logical pool creation/definition parameters

[Jiri Denemark]

On Mon, Mar 27, 2017 at 20:03:42 -0400, John Ferlan wrote:
> 
> 
> On 03/27/2017 05:19 PM, Jiri Denemark wrote:
> > On Mon, Mar 27, 2017 at 13:42:22 -0400, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1398087
> >>
> >> Clean up the virsh man page description for --pool-create-as in order
> >> to better describe how the various arguments are used when creating
> >> (or defining) a logical pool.
> >>
> >> Also modify the storage pool XML parsing algorithm to check for the
> >> mismatched "name" and "source-name".
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/conf/storage_conf.c | 8 
> >>  tools/virsh.pod | 7 +++
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> >> index 585ca71..5213503 100644
> >> --- a/src/conf/storage_conf.c
> >> +++ b/src/conf/storage_conf.c
> >> @@ -760,6 +760,14 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
> >>  if (VIR_STRDUP(ret->source.name, ret->name) < 0)
> >>  goto error;
> >>  }
> >> +if (ret->type == VIR_STORAGE_POOL_LOGICAL &&
> >> +STRNEQ(ret->name, ret->source.name)) {
> >> +virReportError(VIR_ERR_XML_ERROR,
> >> +   _("for a logical pool, the pool name='%s' "
> >> + "must match the pool source name='%s'"),
> >> +   ret->name, ret->source.name);
> >> +goto error;
> > 
> > Wrong indentation...
> > 
> >> +}
> > 
> > but why exactly is this forbidden now? I should be able to create a pool
> > with a (libvirt's) name which differs from the (system's) name of the
> > volume group, shouldn't I? And apparently it used to work while it is
> > not working now after this patch as failing virt-manager builds on
> > ci.centos.org suggest.
> 
> If 'source.name' isn't supplied it defaults to ret->name.  The ret->name
> is supposed to be the Volume Group name (it's the "unique" to the host
> name). Although I suppose it could be something different, but if only
> the name is required in order to define/create the pool, then how does
> one "ensure" that the name provided is the volume group name.

Sure, if only one name is provided, it's OK to use it in both places
(i.e., pool name volume group name).

> Still I certainly suppose someone could do something different, so I
> suppose this part could be reverted.

Definitely, it has to be reverted. Otherwise existing pools which don't
follow this logic will just disappear when libvirtd starts again.

> FWIW: The 'source.name' is used is by the logical backend for various
> vg* and lv* commands. So if it's not the Volume Group name, then
> commands fail (as seen in the bz).

Indeed, source.name has to be to volume group name.

> I really think it's just "smart sense" to make them be the same to
> avoid "confusion".

Perhaps, but it doesn't mean we should forbid pools with different
names. And especially when such pools used to work just fine.

Jirka

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

[libvirt] Entering freeze for libvirt-3.2.0

[Daniel Veillard]

  Hi all,

 as planned I tagged RC1 in git and pushed signed tarball and rpms to the
usual place:

  ftp://libvirt.org/libvirt/

 I had an issue with networking in my limited testing, without time to
really dig it seems my guest image wasn't able to do DNS resolution, that
could be an issue on my side though, and https://ci.centos.org/view/libvirt/
while not all green seems rather positive !

 Please give it a try, I will probably push RC2 on Thursday, and assuming
everything goes fine a release over the week-end,

  thanks !

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH resend V10 01/12] Resctrl: Add some utils functions

[Eli Qiao]

hi Martin

(cc libvir-list)
  
I am a little confused about cat support.  

I am currently rebasing my code on top of pre-cat branch from your private 
github repo, today when I check it you have removed it and create a cat branch 
and there are some related code pushed[1], can I know what ’s your plan for my 
patch set for CAT support ? should I continue my rebasing work? your though?

[1] 
https://github.com/nertpinx/libvirt/commit/c335de47a4efeca87f23e641a93587b1e036e558

Thanks Eli.



--  
Best regards  
Eli

天涯无处不重逢
a leaf duckweed belongs to the sea, where not to meet in life  

Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Friday, 24 March 2017 at 3:42 PM, Martin Kletzander wrote:

> On Fri, Mar 24, 2017 at 09:35:33AM +0800, Eli Qiao wrote:
> >  
> >  
> > --
> > Best regards
> > Eli
> >  
> > 天涯无处不重逢
> > a leaf duckweed belongs to the sea, where not to meet in life
> >  
> > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> >  
> >  
> > On Thursday, 16 March 2017 at 3:52 PM, Eli Qiao wrote:
> >  
> > >  
> > >  
> > > --
> > > Best regards
> > > Eli
> > >  
> > > 天涯无处不重逢
> > > a leaf duckweed belongs to the sea, where not to meet in life
> > >  
> > > Sent with Sparrow (http://www.sparrowmailapp.com/?sig)
> > >  
> > >  
> > > On Wednesday, 15 March 2017 at 7:57 PM, Martin Kletzander wrote:
> > >  
> > > > On Mon, Mar 06, 2017 at 06:06:30PM +0800, Eli Qiao wrote:
> > > > > This patch adds some utils struct and functions to expose resctrl
> > > > > information.
> > > > >  
> > > > > virResCtrlAvailable: if resctrl interface exist on host.
> > > > > virResCtrlGet: get specific type resource control information.
> > > > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > > > > resctrlall[]: an array to maintain resource control information.
> > > > >  
> > > > > Some of host cpu related information methods was added in virhostcpu.c
> > > >  
> > > > So to be able to test all this we need to make a bit different approach.
> > > > I'm not in favour of pushing this without proper tests. Some paths need
> > > > to be configurable, some readings should be unified. Unfortunately lot
> > > > of the code is just copy-paste mess from the past. Fortunately for you,
> > > >  
> > > >  
> > > >  
> > > > I'm working on cleaning this up, at least a little bit, so that we can
> > >  
> > > Good news.
> > > > add the tests easily. I got almost up to the test (I stumbled upon few
> > > > rabbit holes on the way and some clean-ups went wrong along the way), so
> > > > it should be pretty easy for you to modify this code to use what I'll be
> > > > proposing to add. It's not ready yet, but you can start rebasing your
> > > > series on top of my branch pre-cat from my github repo [1]. The commits
> > > > are not very well described right now (for some temporary ones I used
> > > > whatthecommit.com (http://whatthecommit.com), sorry), but I'll fix all 
> > > > that. I'll be updating the
> > > > branch, but it will be done with force pushes, so be careful when
> > > > rebasing on top of newer versions.
> > > >  
> > > > I can do that if you don't want, just let me know so we can coordinate.
> > > of cause we can do some coordinate, but I am glad that you can help on 
> > > this to speed up the progress to merge them, as you know this patch is in 
> > > V10 already, and it has 12 patch set, kinds of hard to doing rebase… :(
> > >  
> > >  
> > > > Just a quick heads-up, there will be virsysfs that will be used for the
> > > > reads, some additional helper functions in virhostcpu and virfile, test
> > > > that scans copy of /sys/devices/system (with that path faked thanks to
> > > > using the aforementioned virsysfs) and outputs capabilities so that we
> > > > can check the capability XML and so on.
> > > >  
> > >  
> > >  
> > >  
> > > Ah, that’s a good news..
> > > >  
> > > > Martin
> > > >  
> > > > [1] https://github.com/nertpinx/libvirt
> >  
> > hi Martin
> >  
> > So, if I understand you correctly , you want all my patch set to rebased on 
> > top of pre-cat branch [1] , I checked that the last commit is 15th March, I 
> > wonder if that ’s ready to merged into master?
> > so that I can start doing the rebasing
> >  
>  
>  
> I forgot to do the usual push, it's updated now. The test fails in one
> occasion, but it's the code's fault, the test is fine. That's the last
> thing I'm looking at now, after that I'll send it to the list.
>  
> Look at the changes and see what you can use, it will help simplifying
> your code a lot, I thing. You can start rebasing on top of that, I'll
> do that as well after it's posted and I'll be either using and modifying
> your patches or maybe doing some myself.
>  
> Martin  

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

Re: [libvirt] [PATCH] storage: Better describe logical pool creation/definition parameters

[Ján Tomko]

On Sat, Mar 25, 2017 at 08:18:46AM -0400, John Ferlan wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1398087

Clean up the virsh man page description for --pool-create-as in order
to better describe how the various arguments are used when creating
(or defining) a logical pool.

Also move the --print-xml to the end of the qualifiers since it's not
properly positionally situated for both --pool-create-as and --pool-define-as.

Finally modify the storage pool XML parsing algorithm to check for the
mismatched "name" and "source-name" as well as a more general if not
provided, then set the default source format.

Signed-off-by: John Ferlan 
---
src/conf/storage_conf.c | 11 +++
tools/virsh.pod | 15 +++
2 files changed, 22 insertions(+), 4 deletions(-)




@@ -757,6 +760,14 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
if (VIR_STRDUP(ret->source.name, ret->name) < 0)
goto error;
}
+if (ret->type == VIR_STORAGE_POOL_LOGICAL &&
+STRNEQ(ret->name, ret->source.name)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("for a logical pool, the pool name='%s' "
+ "must match the pool source name='%s'"),
+   ret->name, ret->source.name);


Why?

Jan


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