Re: [libvirt] [PATCH 01/14] virstoragefile: Introduce virStoragePRDef

[John Ferlan]

On 01/18/2018 11:04 AM, Michal Privoznik wrote:
> This is a definition that holds information on SCSI persistent
> reservation settings. The XML part looks like this:
> 
>   
> 
>   
> 
> If @managed is set to 'yes' then the  is not parsed.
> This design was agreed on here:
> 
> https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html
> 
> Signed-off-by: Michal Privoznik 
> ---
>  docs/formatdomain.html.in  |  25 +++-
>  docs/schemas/domaincommon.rng  |  19 +--
>  docs/schemas/storagecommon.rng |  34 +
>  src/conf/domain_conf.c |  36 +
>  src/libvirt_private.syms   |   3 +
>  src/util/virstoragefile.c  | 148 
> +
>  src/util/virstoragefile.h  |  15 +++
>  .../disk-virtio-scsi-reservations-not-managed.xml  |  40 ++
>  .../disk-virtio-scsi-reservations.xml  |  38 ++
>  .../disk-virtio-scsi-reservations-not-managed.xml  |   1 +
>  .../disk-virtio-scsi-reservations.xml  |   1 +
>  tests/qemuxml2xmltest.c|   4 +
>  12 files changed, 348 insertions(+), 16 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/disk-virtio-scsi-reservations-not-managed.xml
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml
>  create mode 12 
> tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations-not-managed.xml
>  create mode 12 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml
> 

Before digging too deep into this...

 - I assume we're avoiding  iSCSI mainly because those
reservations would take place elsewhere, safe assumption?

 - What about using lun's from a storage pool (and what could become
your favorite, NPIV devices ;-))

   
 
 
 
   

 - What about 's?

   

   but not iSCSI or vHost hostdev's. I think that creates the SCSI
generic LUN, but it's been a while since I've thought about the
terminology used for hostdev's...

   I also have this faint recollection of PR related to sgio filtering
and perhaps even rawio, but dredging that back up could send me down the
path of some "downstream only" type bz's. Although searching on just
VIR_DOMAIN_DISK_DEVICE_LUN does bring up qemuSetUnprivSGIO.

And finally... I assume there is one qemu-pr-manager (qemu.conf changes
eventually)... Eventually there's magic that allows/adds per domain
*and* per LUN some socket path. If libvirt provided it's generated via
the domain temporary directory; however, what's not really clear is how
that unmanaged path really works.  Need a virtual whiteboard...

I only commented at this first patch for now. I did briefly scan the
others, but too many questions here to dig deep into those. I did run
the entire series through my Coverity checker - there's 3 things that
get tagged in later patches.

John

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d272cc1ba..23c5f071e 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2457,7 +2457,10 @@
>/disk
>disk type='block' device='lun'
>  driver name='qemu' type='raw'/
> -source dev='/dev/sda'/
> +source dev='/dev/sda'
> +  reservations enabled='yes' managed='no'
> +source type='unix' path='/path/to/qemu-pr-helper' 
> mode='client'/
> +  /reservations
>  target dev='sda' bus='scsi'/
>  address type='drive' controller='0' bus='0' target='3' unit='0'/
>/disk
> @@ -2819,6 +2822,26 @@
>  Storage Encryption
>  page for more information.
>
> +  reservations
> +  Since libvirt 3.10.0, the

Will be 4.1.0 at least...

> +reservations can be a sub-element of the
> +source element for storage sources (QEMU driver 
> only).
> +If present (and enabled) it enabled persistent reservations for

Doesn't read well - even with the second enabled changed to enables.

Furthermore what's the purpose of enabled=no? Isn't that just like
removing ? If so, then I think enabled just gets removed
and the "managed" is the only required attribute. Besides you don't have
a "enabled='no'" example.  And, no, sorry I didn't go back through the
entire "New QEMU daemon for persistent reservations" discussion that
started in August to see if this was discussed there.

Looking at virStoragePRDefFormat when enabled='no' anything anyone set
for managed would be lost (e.g. not printed). IOW: by changing from
enabled=yes and managed=no to enabled=no, then everything in the
 subelement is lost.

> +SCSI based disks. The element has one mandatory attribute
> +enabled with accepted values yes and

NB: Other uses go with "yes" and "no" not the code wrapped tag.

> +no. If the feature is enabled, then there's another
> +mandatory attribute managed (accepted values 

Re: [libvirt] [PATCH] apparmor: allow libvirt to send term signal to unconfined

[Jamie Strandboge]

On Wed, 2018-01-24 at 10:41 +0100, intrigeri wrote:
> Hi,
> 
> 
> Guido Günther:
> > --- a/examples/apparmor/usr.sbin.libvirtd
> > +++ b/examples/apparmor/usr.sbin.libvirtd
> > @@ -63,7 +63,7 @@
> >signal (send) peer=/usr/sbin/dnsmasq,
> >signal (read, send) peer=libvirt-*,
> > -  signal (send) set=("kill") peer=unconfined,
> > +  signal (send) set=("kill", "term") peer=unconfined,
> 
LGTM too. +1 to apply.

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] docs: Add missing element encryption description term entry

[John Ferlan]

Missed adding the "encryption" description term entry to the list
of possible sub-elements for disk source. The description details
were there, just not the tag.

Signed-off-by: John Ferlan 
---
 Found this while looking at Michal's persistent reservations series.

 Pushing as trivial - it doesn't cause a conflict with that series.

 docs/formatdomain.html.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 41ba4d41e..cf0654497 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2824,6 +2824,7 @@
 attribute matching the key that was specified in the
 secret object.
   
+  encryption
   Since libvirt 3.9.0, the
 encryption can be a sub-element of the
 source element for encrypted storage sources.
-- 
2.13.6

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

[libvirt] [PATCH] resctl: stub out functions with Linux-only APIs used

[Daniel P . Berrangé]

The flock() function and d_type field in struct dirent are not portable
to the mingw platform.

Signed-off-by: Daniel P. Berrangé 
---

  * Pushed as CI build fix

 src/util/virresctrl.c | 47 +++
 1 file changed, 47 insertions(+)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index e252aefe31..754820ee46 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -295,6 +295,7 @@ virResctrlAllocNew(void)
 
 
 /* Common functions */
+#ifdef __linux__
 static int
 virResctrlLockInternal(int op)
 {
@@ -321,6 +322,20 @@ virResctrlLockWrite(void)
 return virResctrlLockInternal(LOCK_EX);
 }
 
+#else
+
+static inline int
+virResctrlLockWrite(void)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("resctrlfs not supported on this platform"));
+return -1;
+}
+
+#endif
+
+
+
 
 static int
 virResctrlUnlock(int fd)
@@ -328,6 +343,7 @@ virResctrlUnlock(int fd)
 if (fd == -1)
 return 0;
 
+#ifdef __linux__
 /* The lock gets unlocked by closing the fd, which we need to do anyway in
  * order to clean up properly */
 if (VIR_CLOSE(fd) < 0) {
@@ -338,6 +354,7 @@ virResctrlUnlock(int fd)
 virReportSystemError(errno, "%s", _("Cannot unlock resctrlfs"));
 return -1;
 }
+#endif /* ! __linux__ */
 
 return 0;
 }
@@ -369,6 +386,8 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
 }
 
 
+#ifdef __linux__
+
 int
 virResctrlGetInfo(virResctrlInfoPtr resctrl)
 {
@@ -495,6 +514,18 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
 return ret;
 }
 
+#else /* ! __linux__ */
+
+int
+virResctrlGetInfo(virResctrlInfoPtr resctrl ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Cache tune not supported on this platform"));
+return -1;
+}
+
+#endif /* ! __linux__ */
+
 
 int
 virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
@@ -632,6 +663,8 @@ virResctrlAllocGetType(virResctrlAllocPtr resctrl,
 }
 
 
+#ifdef __linux__
+
 static int
 virResctrlAllocUpdateMask(virResctrlAllocPtr resctrl,
   unsigned int level,
@@ -659,6 +692,8 @@ virResctrlAllocUpdateMask(virResctrlAllocPtr resctrl,
 return virBitmapCopy(a_type->masks[cache], mask);
 }
 
+#endif
+
 
 static int
 virResctrlAllocUpdateSize(virResctrlAllocPtr resctrl,
@@ -878,6 +913,8 @@ virResctrlAllocFormat(virResctrlAllocPtr resctrl)
 }
 
 
+#ifdef __linux__
+
 static int
 virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
  virResctrlAllocPtr alloc,
@@ -1180,7 +1217,17 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl)
 goto cleanup;
 }
 
+#else /* ! __linux__ */
+
+virResctrlAllocPtr
+virResctrlAllocGetUnused(virResctrlInfoPtr resctrl ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Cache tune not supported on this platform"));
+return NULL;
+}
 
+#endif /* ! __linux__ */
 
 static int
 virResctrlAllocSetMask(virResctrlAllocPerTypePtr a_type,
-- 
2.14.3

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

Re: [libvirt] [REBASE PATCH v2 2/9] qemu: Introduce qemuProcessHandleDumpCompleted

[Jiri Denemark]

On Fri, Jan 19, 2018 at 14:53:09 -0500, John Ferlan wrote:
> Add a couple of booleans to mark when there's a dump completion event
> waiting and when a dump completed event has been received.
> 
> To ensure the dump completed event from a non memory-only dump doesn't
> cause the a dump completed event to be fired, only broadcast if there's
> a completion event waiting.

I think you can just drop job->dumpCompletion as it doesn't seem to be
any useful. You set job->dumpCompleted even if dumpCompletion is false,
it's just guarding the broadcast, which is harmless. If you were
concerned about DUMP_COMPLETED event being send by some other command
than the one we call from qemuDumpToFd, you could just ignore the event
if the dump job was running, which you should probably do anyway (see
qemuProcessHandleMigration*) to make sure you job->dumpCompleted is not
unexpectedly set to true before a dump job starts.

Jirka

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

Re: [libvirt] [REBASE PATCH v2 1/9] qemu: Add support for DUMP_COMPLETED event

[Jiri Denemark]

On Fri, Jan 19, 2018 at 14:53:08 -0500, John Ferlan wrote:
> The event is fired when the domain memory only dump completes.
> 
> Wire up the code to extract and send along the status.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_monitor.c  | 18 ++
>  src/qemu/qemu_monitor.h  | 19 +++
>  src/qemu/qemu_monitor_json.c | 31 +++
>  3 files changed, 68 insertions(+)
...
> +static void
> +qemuMonitorJSONHandleDumpCompleted(qemuMonitorPtr mon,
> +   virJSONValuePtr data)
> +{
> +const char *statusstr;
> +qemuMonitorDumpStatus status;
> +virJSONValuePtr result;
> +
> +if (!(result = virJSONValueObjectGetObject(data, "result"))) {
> +VIR_WARN("missing result in dump completed event");
> +return;
> +}
> +
> +if (!(statusstr = virJSONValueObjectGetString(result, "status"))) {
> +VIR_WARN("missing status string in dump completed event");
> +return;
> +}
> +
> +status = qemuMonitorDumpStatusTypeFromString(statusstr);
> +if (status < 0) {
> +VIR_WARN("invalid status string '%s' in dump completed event",
> + statusstr);
> +return;
> +}
> +
> +qemuMonitorEmitDumpCompleted(mon, status);

According to qapi-schema the DUMP_COMPLETED event may contain an error
string. Don't we want to consume it here too?

Jirka

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

Re: [libvirt] [PATCH] qemu: Add USB and memory balloon by default for aarch64/virt guests

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 05:45:51PM +0100, Andrea Bolognani wrote:
> Basically all existing guest types, regardless of the architectur,
> get both a USB controller and a virtio memory balloon by default.
> 
> s390 guests are an exception, for the very good reason that they
> don't support USB at all; the other exception is aarch64/virt
> guests, but in the latter case isn't a compelling reason for them
> to deviate from the widely adopted convention, especially since:
> 
>   * x86/q35 guests, which aarch64/virt guests are for the most
> part identical to, add these devices by default;
>   * it's trivial to opt out of both default devices by setting
> model='none';

Except that this requires code changes to downstream applications to
actually do this now, otherwise guests that they were expecting to
not have USB for, now suddenly get it.

>   * higher level applications such as Nova expect at least the
> USB controller to be present.

This doesn't really help nova in practice, because it needs to operate
correctly with pre-existing libbvirt releases, and even on x86 it should
not be relying on the default USB1 controller, but rather adding a USB2
or USB3 controller.

> So add it by default for newly-defined guests. Existing guests
> will, of course, be left unchanged.

That is still harmful, because an existing mgmt application release that
runs on new libvirt has its guest configs suddenly changed, especially
if using transient guests.

> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1538637

I consider that bug wontfix.  It is just exchanging one set of problems
for a different set of problems.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] tests: Clean up GIC test cases

[Andrea Bolognani]

These test cases are supposed to verify GIC support works as
expected, and shouldn't concern themselves with other features;
we can trim them down significantly, and make them less likely
to need updating after unrelated changes.

Signed-off-by: Andrea Bolognani 
---
 tests/qemuxml2argvdata/aarch64-gic-default.xml| 8 ++--
 tests/qemuxml2argvdata/aarch64-gic-host.xml   | 2 ++
 tests/qemuxml2argvdata/aarch64-gic-invalid.xml| 8 ++--
 tests/qemuxml2argvdata/aarch64-gic-none-tcg.xml   | 2 +-
 tests/qemuxml2argvdata/aarch64-gic-none.xml   | 8 ++--
 tests/qemuxml2argvdata/aarch64-gic-not-arm.xml| 8 ++--
 tests/qemuxml2argvdata/aarch64-gic-not-virt.xml   | 8 ++--
 tests/qemuxml2argvdata/aarch64-gic-v2.xml | 2 ++
 tests/qemuxml2argvdata/aarch64-gic-v3.xml | 2 ++
 tests/qemuxml2xmloutdata/aarch64-gic-none-tcg.xml | 1 +
 10 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/tests/qemuxml2argvdata/aarch64-gic-default.xml 
b/tests/qemuxml2argvdata/aarch64-gic-default.xml
index b219972b3..accfbb0af 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-default.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-default.xml
@@ -2,21 +2,17 @@
   aarch64test
   6ba410c5-1e5c-4d57-bee7-2228e7ffa32f
   1048576
-  1048576
   1
   
 hvm
-
   
   
 
   
   
-  
-  destroy
-  restart
-  destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-host.xml 
b/tests/qemuxml2argvdata/aarch64-gic-host.xml
index b14d14281..561f6edc6 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-host.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-host.xml
@@ -18,5 +18,7 @@
   destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-invalid.xml 
b/tests/qemuxml2argvdata/aarch64-gic-invalid.xml
index 1cf9ea88d..98566b86d 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-invalid.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-invalid.xml
@@ -2,21 +2,17 @@
   aarch64test
   6ba410c5-1e5c-4d57-bee7-2228e7ffa32f
   1048576
-  1048576
   1
   
 hvm
-
   
   
 
   
   
-  
-  destroy
-  restart
-  destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-none-tcg.xml 
b/tests/qemuxml2argvdata/aarch64-gic-none-tcg.xml
index 0aa33dbec..64745f3e0 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-none-tcg.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-none-tcg.xml
@@ -5,13 +5,13 @@
   1
   
 hvm
-
   
   
 cortex-a57
   
   
 /usr/bin/qemu-system-aarch64
+
 
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-none.xml 
b/tests/qemuxml2argvdata/aarch64-gic-none.xml
index 272d0c857..302c132a3 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-none.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-none.xml
@@ -2,18 +2,14 @@
   aarch64test
   6ba410c5-1e5c-4d57-bee7-2228e7ffa32f
   1048576
-  1048576
   1
   
 hvm
-
   
   
-  
-  destroy
-  restart
-  destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-not-arm.xml 
b/tests/qemuxml2argvdata/aarch64-gic-not-arm.xml
index 3b907bc41..e33bca0ef 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-not-arm.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-not-arm.xml
@@ -2,21 +2,17 @@
   aarch64test
   6ba410c5-1e5c-4d57-bee7-2228e7ffa32f
   1048576
-  1048576
   1
   
 hvm
-
   
   
 
   
   
-  
-  destroy
-  restart
-  destroy
   
 /usr/bin/qemu-system-ppc64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-not-virt.xml 
b/tests/qemuxml2argvdata/aarch64-gic-not-virt.xml
index 256664ed8..12148d99e 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-not-virt.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-not-virt.xml
@@ -2,21 +2,17 @@
   aarch64test
   6ba410c5-1e5c-4d57-bee7-2228e7ffa32f
   1048576
-  1048576
   1
   
 hvm
-
   
   
 
   
   
-  
-  destroy
-  restart
-  destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-v2.xml 
b/tests/qemuxml2argvdata/aarch64-gic-v2.xml
index 8b9983752..f451c87b7 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-v2.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-v2.xml
@@ -18,5 +18,7 @@
   destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2argvdata/aarch64-gic-v3.xml 
b/tests/qemuxml2argvdata/aarch64-gic-v3.xml
index bde94e16c..f3a6b9283 100644
--- a/tests/qemuxml2argvdata/aarch64-gic-v3.xml
+++ b/tests/qemuxml2argvdata/aarch64-gic-v3.xml
@@ -18,5 +18,7 @@
   destroy
   
 /usr/bin/qemu-system-aarch64
+
+
   
 
diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-none-tcg.xml 
b/tests/qemuxml2xmloutdata/aarch64-gic-none-tcg.xml
index a0cd0b768..b97843970 100644
--- a/tests/qemuxml2xmloutdata/aarch64-gic-none-tcg.xml
+++ b/tests/qemuxml2xmloutdata/aarch64-gic-none-tcg.xml
@@ -20,6 +20,7 @@
   destroy
   
 

[libvirt] [PATCH] qemu: Add USB and memory balloon by default for aarch64/virt guests

[Andrea Bolognani]

Basically all existing guest types, regardless of the architectur,
get both a USB controller and a virtio memory balloon by default.

s390 guests are an exception, for the very good reason that they
don't support USB at all; the other exception is aarch64/virt
guests, but in the latter case isn't a compelling reason for them
to deviate from the widely adopted convention, especially since:

  * x86/q35 guests, which aarch64/virt guests are for the most
part identical to, add these devices by default;
  * it's trivial to opt out of both default devices by setting
model='none';
  * higher level applications such as Nova expect at least the
USB controller to be present.

So add it by default for newly-defined guests. Existing guests
will, of course, be left unchanged.

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

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 34 +-
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6b4bd3cca..f1c3b3d1e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2630,7 +2630,8 @@ qemuDomainDefAddImplicitInputDevice(virDomainDef *def)
 
 static int
 qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
-   virQEMUCapsPtr qemuCaps)
+   virQEMUCapsPtr qemuCaps,
+   unsigned int parseFlags)
 {
 bool addDefaultUSB = true;
 int usbModel = -1; /* "default for machinetype" */
@@ -2680,10 +2681,33 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
 
 case VIR_ARCH_ARMV7L:
 case VIR_ARCH_AARCH64:
-addDefaultUSB = false;
-addDefaultMemballoon = false;
-if (qemuDomainIsVirt(def))
+if (qemuDomainIsVirt(def)) {
+/* All mach-virt guests get a PCIe Root, if supported by
+ * the QEMU binary */
 addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
+}
+
+if (qemuDomainIsVirt(def) &&
+parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
+/* In addition to PCIe Root, newly-defined mach-virt guests
+ * also get a couple more devices so that they're more similar
+ * to guests on other architectures, notably x86/q35:
+ *
+ *   1) a USB3 controller, if supported by the QEMU binary;
+ *   2) a virtio memory balloon, as per the defaults defined
+ *  above.
+ */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+usbModel = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+else
+addDefaultUSB = false;
+} else {
+/* Other ARM guests (and existing mach-virt guests, in order
+ * to preserve guest ABI compatibility) don't get a PCIe Root,
+ * a USB controller or a memory balloon */
+addDefaultUSB = false;
+addDefaultMemballoon = false;
+}
 break;
 
 case VIR_ARCH_PPC64:
@@ -3187,7 +3211,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
 goto cleanup;
 }
 
-if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0)
+if (qemuDomainDefAddDefaultDevices(def, qemuCaps, parseFlags) < 0)
 goto cleanup;
 
 if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
-- 
2.14.3

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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 11:13:40AM +0100, Peter Krempa wrote:
> On Mon, Jan 22, 2018 at 13:57:07 +, Daniel Berrange wrote:
> > On Mon, Jan 22, 2018 at 02:20:52PM +0100, Peter Krempa wrote:
> > > On Mon, Jan 22, 2018 at 13:06:28 +, Daniel Berrange wrote:
> > > > On Mon, Jan 22, 2018 at 01:20:12PM +0100, Peter Krempa wrote:
> > > > > On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
> > > > > > This extends the update hook so that it enforces a requirement to 
> > > > > > have a
> > > > > > Signed-off-by line in every commit message. This can be optionally
> > > > > > turned off in individual repos by setting the 
> > > > > > "hooks.allowmissingsob"
> > > > > > git config variable.
> > > > > > 
> > > > > > Signed-off-by: Daniel P. Berrange 
> > > > > > ---
> > > > > >  update | 16 +++-
> > > > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> [..]
> 
> > > The sign-off by itself (whithout cryptographic signature) is just
> > > pointless. Validity with a cryptographic signature from drive-by
> > > contributors can still be unproven, but at least you don't get
> > > impersonation.
> > 
> > I think these are two different axis. The sob isn't trying to address the
> > question of impersonation. It obviously has as a starting point that you
> > accept the identity of the submitter to some degree. I accept that if you
> > have cryptographically signed patches, that would give a stronger
> > validation of identity, but there's never any absolutes. So not having
> > a crypto signature doesn't make the sob invalid.
> 
> In that case basically nothing changes, since if we are going to use
> this to be safe from licensing disputes, the reviewer/commiter still
> needs to make sure that the code complies with our licensing. Asserting
> the signoff changes nothing in that regard
> 
> > 
> > > If everything is signed off, nothing really is.
> > 
> > I don't really see that.
> > 
> > > NACK still stands.
> > 
> > You are nacking something that you've accepted above will have no negative
> > impact on your work, but has potentially significant upside to the project.
> > That is very disappointing.
> 
> I think that by doing this we'll put too much false hope into the
> "potentially significant upside". I just hope it will not bite us.
> 
> Anyone can assert, or sign-off anything [1].
> 
> Given the overwhelmingly positive approach to this retract my NACK, the
> only thing that will change in general is that my commits will grow one
> line.

Thanks, I appreciate that.

FYI, i'm not going to push the hook right now, since I'm on holiday for
3 days and its always bad to do changes to dev workflow before going
away :-)  I'll do it next week when i return

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 2/5] conf: Add pSeries features

[Andrea Bolognani]

On Thu, 2018-01-25 at 15:03 +, Daniel P. Berrangé wrote:
> On Tue, Jan 23, 2018 at 04:45:01PM +0100, Andrea Bolognani wrote:
> > We're going to introduce a number of optional, pSeries-specific
> > features, so we want to have a dedicated sub-element to avoid
> > cluttering the  element.
> 
> I don't think we want todo this as all. Pretty much *every* single
> existing  element is architecture specific. Adding a nested
> level just for pseries is making it different from existing practice
> and I don't see any benefit to that.

Well, there's going to be a bunch of them added pretty soon (six
or so) plus probably more down the line, so that's already quite
a bit of clutter. My reasoning was to try and organize them the
way we already do with  and  capabilities - even
though pSeries is not a hypervisor per se, you can kinda see
sPAPR as a specification implemented by various hypervisors, like
PowerVM and in our case QEMU/KVM. But if you think this effort is
misguided and they belong to the top-level  element,
then I'm okay with that too.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 2/5] conf: Add pSeries features

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 05:12:49PM +0100, Andrea Bolognani wrote:
> On Thu, 2018-01-25 at 15:03 +, Daniel P. Berrangé wrote:
> > On Tue, Jan 23, 2018 at 04:45:01PM +0100, Andrea Bolognani wrote:
> > > We're going to introduce a number of optional, pSeries-specific
> > > features, so we want to have a dedicated sub-element to avoid
> > > cluttering the  element.
> > 
> > I don't think we want todo this as all. Pretty much *every* single
> > existing  element is architecture specific. Adding a nested
> > level just for pseries is making it different from existing practice
> > and I don't see any benefit to that.
> 
> Well, there's going to be a bunch of them added pretty soon (six
> or so) plus probably more down the line, so that's already quite
> a bit of clutter. My reasoning was to try and organize them the
> way we already do with  and  capabilities - even
> though pSeries is not a hypervisor per se, you can kinda see
> sPAPR as a specification implemented by various hypervisors, like
> PowerVM and in our case QEMU/KVM. But if you think this effort is
> misguided and they belong to the top-level  element,
> then I'm okay with that too.

I guess where the nesting makes sense is if there's a chance of having
namespace collision between features.  eg if both kvm and hyperv
had a feature called "pvspinlocks", you might want to enable them
separately, so the nesting is important there.

If you do think the pSeries nesting is important in this respect, can
you still leave the existing "hpt" feature un-nested for sake of
full back-compat - just nest new features.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 0/2] vsh: Print help for the aliased cmd rather than the alias itself

[Erik Skultety]

On Thu, Jan 25, 2018 at 04:49:38PM +0100, Michal Privoznik wrote:
> On 01/25/2018 04:22 PM, Erik Skultety wrote:
> > The alias doesn't have any definition rather than a reference for the 
> > aliased
> > command. Therefore when printing help, we have to print help for the aliased
> > command, otherwise our faith is going to be met with SIGSEGV.
> >
> > Erik Skultety (2):
> >   vsh: Drop redundant definition searches from vshCmd{def,Grp}Help
> >   vsh: Cmd aliases lookups should return results for the aliased command
> >
> >  tools/vsh.c | 45 +
> >  tools/vsh.h |  4 ++--
> >  2 files changed, 19 insertions(+), 30 deletions(-)
> >
>
> ACK series

Pushed, thanks.

Erik

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

Re: [libvirt] [PATCH 0/2] vsh: Print help for the aliased cmd rather than the alias itself

[Michal Privoznik]

On 01/25/2018 04:22 PM, Erik Skultety wrote:
> The alias doesn't have any definition rather than a reference for the aliased
> command. Therefore when printing help, we have to print help for the aliased
> command, otherwise our faith is going to be met with SIGSEGV.
> 
> Erik Skultety (2):
>   vsh: Drop redundant definition searches from vshCmd{def,Grp}Help
>   vsh: Cmd aliases lookups should return results for the aliased command
> 
>  tools/vsh.c | 45 +
>  tools/vsh.h |  4 ++--
>  2 files changed, 19 insertions(+), 30 deletions(-)
> 

ACK series

Michal

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

Re: [libvirt] [PATCH v2 4/4] news: qemu: use arp table of host to get the IP address of guests

[Michal Privoznik]

On 01/24/2018 05:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  docs/news.xml | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index b9e04c632..105917f4d 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -47,6 +47,11 @@
>interfaces, NWFilters, and so on).
>  
>
> +  
> +
> +  qemu: use arp table of host to get the IP address of guests
> +
> +  
>  
>  
>  
> 

Could be more verbose, but it's okayish.

Michal

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

Re: [libvirt] [PATCH v2 2/4] qemu: introduce qemuARPGetInterfaces to get IP from host's arp table

[Michal Privoznik]

On 01/24/2018 05:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> introduce VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP to get ip address
> of VM from the output of /proc/net/arp
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/qemu/qemu_driver.c   | 75 
> 
>  2 files changed, 76 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 4048acf38..38e2d9a3e 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -4665,6 +4665,7 @@ typedef virMemoryParameter *virMemoryParameterPtr;
>  typedef enum {
>  VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE = 0, /* Parse DHCP lease file */
>  VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT = 1, /* Query qemu guest agent 
> */
> +VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP = 2, /* Query ARP tables */

You should document the flag in src/libvirt-domain.c in
virDomainInterfaceAddresses documentation and also mention all the
limitations.

>  
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a203c9297..e31a74261 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -160,6 +160,9 @@ static int qemuGetDHCPInterfaces(virDomainPtr dom,
>   virDomainObjPtr vm,
>   virDomainInterfacePtr **ifaces);
>  
> +static int qemuARPGetInterfaces(virDomainObjPtr vm,
> +virDomainInterfacePtr **ifaces);
> +
>  static virQEMUDriverPtr qemu_driver;
>  
>  
> @@ -20384,6 +20387,10 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
>  
>  break;
>  
> +case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_ARP:
> +ret = qemuARPGetInterfaces(vm, ifaces);
> +break;
> +
>  default:
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> _("Unknown IP address data source %d"),
> @@ -20494,6 +20501,74 @@ qemuGetDHCPInterfaces(virDomainPtr dom,
>  }
>  
>  
> +static int
> +qemuARPGetInterfaces(virDomainObjPtr vm,
> + virDomainInterfacePtr **ifaces)
> +{
> +size_t i, j;
> +size_t ifaces_count = 0;
> +int ret = -1;
> +char macaddr[VIR_MAC_STRING_BUFLEN];
> +virDomainInterfacePtr *ifaces_ret = NULL;
> +virDomainInterfacePtr iface = NULL;
> +
> +virArpTablePtr table;
> +if (VIR_ALLOC(table) < 0)
> +goto error;

The empty line should go between block of variable declaration and the
code block.

> +if (virGetArpTable() < 0)
> +goto cleanup;
> +
> +for (i = 0; i < vm->def->nnets; i++) {
> +if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> +continue;
> +
> +virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
> +virArpTableEntry entry;
> +for (j = 0; j < table->n; j++) {
> +entry = table->t[j];
> +if (STREQ(entry.mac, macaddr)) {
> +if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
> +goto error;
> +
> +if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0)
> +goto error;
> +
> +iface = ifaces_ret[ifaces_count - 1];
> +iface->naddrs = 1;
> +if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0)
> +goto error;
> +
> +if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 
> 0)
> +goto cleanup;

Interesting. If VIR_ALLOC_N() fails you jump to error, vir STRDUP fails
you jump to cleanup. That is not right. @iface is leaked if this STRDUP
fails. Just drop the error label and move it to cleanup. In case of
success @ifaces_ret is NULL so virDomainInterfaceFree() is not called
(btw might reset ifaces_count to zero).

> +
> +if (VIR_STRDUP(iface->hwaddr, macaddr) < 0)
> +goto cleanup;
> +
> +if (VIR_STRDUP(iface->addrs->addr, entry.ipaddr) < 0)
> +goto cleanup;

I wonder if there's something we can do with prefix. I mean, the arp
table has no knowledge of it so probably it's okay to leave it as is (=
set to 0). Just wanted to point this fact out.

> +}
> +}
> +}
> +
> +*ifaces = ifaces_ret;
> +ifaces_ret = NULL;
> +ret = ifaces_count;
> +
> + cleanup:
> +virArpTableFree(table);
> +return ret;
> +
> + error:
> +if (ifaces_ret) {
> +for (i = 0; i < ifaces_count; i++)
> +virDomainInterfaceFree(ifaces_ret[i]);
> +}
> +VIR_FREE(ifaces_ret);
> +
> +goto cleanup;
> +}
> +
> +

Michal

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

Re: [libvirt] [PATCH v2 1/4] util: introduce helper to parse /proc/net/arp

[Michal Privoznik]

On 01/24/2018 05:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> introduce helper to parse /proc/net/arp and
> store it in struct virArpTable.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virmacaddr.c| 67 
> 
>  src/util/virmacaddr.h| 18 +
>  3 files changed, 87 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index bc8cc1fba..26385a2e9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2133,6 +2133,8 @@ virLogVMessage;
>  
>  
>  # util/virmacaddr.h
> +virArpTableFree;
> +virGetArpTable;
>  virMacAddrCmp;
>  virMacAddrCmpRaw;
>  virMacAddrCompare;

See how these APIs are named? virModuleAction. So in your case it should
be virArpTableGet(), virArpTableFree().

> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 409fdc34d..540bdbbaa 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -27,10 +27,15 @@
>  #include 
>  
>  #include "c-ctype.h"
> +#include "viralloc.h"
> +#include "virfile.h"
>  #include "virmacaddr.h"
>  #include "virrandom.h"
> +#include "virstring.h"
>  #include "virutil.h"
>  
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
>  static const unsigned char virMacAddrBroadcastAddrRaw[VIR_MAC_BUFLEN] =
>  { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>  
> @@ -257,3 +262,65 @@ virMacAddrIsBroadcastRaw(const unsigned char 
> s[VIR_MAC_BUFLEN])
>  {
>  return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
>  }
> +
> +int
> +virGetArpTable(virArpTablePtr *table)
> +{
> +#define PROC_NET_ARP"/proc/net/arp"

Since the string is used at exactly one place this #define feels redundant.

Also, the function could allocate the returned table too. In fact it
could return that instead of int:

virArpTablePtr virArpTableGet(void);

> +FILE *fp = NULL;
> +char line[1024];
> +int num = 0;
> +int ret = -1;
> +
> +if (!(fp = fopen(PROC_NET_ARP, "r")))
> +goto cleanup;

Problem with this is that if fopen() fails no error is reported and thus
caller doesn't know what went wrong. Other functions that you call here
do set error (e.g. VIR_REALLOC_N, VIR_STRDUP).

So what happens if this function is called on FreeBSD where ARP table is
not exposed through /proc/net/arp? I guess we need two versions of this
function: Linux and non-Linux one (which could do one thing - report
ENOSUPP error). Look at virNetDevTapInterfaceStats().

> +
> +while (fgets(line, sizeof(line), fp)) {
> +char ip[32], mac[32], dev_name[32], hwtype[32],
> + flags[32], mask[32], nouse[32];
> +
> +if (STRPREFIX(line, "IP address"))
> +continue;
> +
> +num++;

Small nit, you can do this at the end of the loop body. That way you
don't need to have all those (num - 1). And for realloc go with
VIR_REALLOC_N(,num + 1);
This is actually more correct too because if REALLOC fails, num contains
wrong number of records (not that it causes problems, it's just semantic).

> +if (VIR_REALLOC_N((*table)->t, num) < 0)
> +goto cleanup;
> +(*table)->n = num;
> +/* /proc/net/arp looks like:
> + * 172.16.17.254  0x1 0x2  e4:68:a3:8d:ed:d3  *   enp3s0
> + */
> +sscanf(line, "%[0-9.]%[ ]%[^ ]%[ ]%[^ ]%[ ]%[^ ]%[ ]%[^ ]%[ ]%[^ 
> \t\n]",
> +   ip, nouse,
> +   hwtype, nouse,
> +   flags, nouse,
> +   mac, nouse,
> +   mask, nouse,
> +   dev_name);
> +
> +
> +if (VIR_STRDUP((*table)->t[num - 1].ipaddr, ip) < 0)
> +goto cleanup;
> +if (VIR_STRDUP((*table)->t[num - 1].mac, mac) < 0)
> +goto cleanup;
> +if (VIR_STRDUP((*table)->t[num - 1].dev_name, dev_name) < 0)
> +goto cleanup;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FORCE_FCLOSE(fp);
> +return ret;
> +}
> +
> +void
> +virArpTableFree(virArpTablePtr table)
> +{
> +size_t i;
> +for (i = 0; i < table->n; i++) {
> +VIR_FREE(table->t[i].ipaddr);
> +VIR_FREE(table->t[i].mac);
> +VIR_FREE(table->t[i].dev_name);
> +}
> +VIR_FREE(table);
> +}
> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
> index ef4285d63..eb18092d1 100644
> --- a/src/util/virmacaddr.h
> +++ b/src/util/virmacaddr.h
> @@ -40,6 +40,22 @@ struct _virMacAddr {
> false otherwise. */
>  };
>  
> +typedef struct _virArpTableEntry virArpTableEntry;
> +typedef virArpTableEntry *virArpTableEntryPtr;
> +typedef struct _virArpTable virArpTable;
> +typedef virArpTable *virArpTablePtr;

Don't we want to have these in separate file? I mean, we can have
virarptable.[ch] because these are not really MAC related, are they?

Michal

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

Re: [libvirt] [PATCH v2 3/4] virsh: add --source arp to domifaddr

[Michal Privoznik]

On 01/24/2018 05:09 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> We can use:
>   domifaddr f26-cloud --source arp
> to get the address.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  tools/virsh-domain-monitor.c | 2 ++
>  tools/virsh.pod  | 7 ---
>  2 files changed, 6 insertions(+), 3 deletions(-)

ACK

Michal

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

[libvirt] [PATCH 2/2] vsh: Cmd aliases lookups should return results for the aliased command

[Erik Skultety]

Unfortunately, we have a number of aliases in virsh and even though
these are not visible any more, we have to support them. The problem is
that when trying to print help for the alias, we get SIGSEGV because
there isn't any @def structure anymore and we need to query the command
being aliased instead.

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

Signed-off-by: Erik Skultety 
---
 tools/vsh.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/vsh.c b/tools/vsh.c
index 761d2ec3a..37c292a03 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -3194,6 +3194,8 @@ cmdHelp(vshControl *ctl, const vshCmd *cmd)
 }
 
 if ((def = vshCmddefSearch(name))) {
+if (def->flags & VSH_CMD_FLAG_ALIAS)
+def = vshCmddefSearch(def->alias);
 return vshCmddefHelp(ctl, def);
 } else if ((grp = vshCmdGrpSearch(name))) {
 return vshCmdGrpHelp(ctl, grp);
-- 
2.13.6

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

[libvirt] [PATCH 1/2] vsh: Drop redundant definition searches from vshCmd{def, Grp}Help

[Erik Skultety]

These helpers are called from a single place only - cmdHelp wrapper and
just before the wrapper invokes the helpers, it performs the search,
either for command group or for the command itself, except the result is
discarded and the helper therefore needs to do it again. Drop this
inefficient handling and pass the @def structure rather than a name,
thus preventing the helper from needing to perform the search again.

Signed-off-by: Erik Skultety 
---
 tools/vsh.c | 43 +++
 tools/vsh.h |  4 ++--
 2 files changed, 17 insertions(+), 30 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index 058df7ef6..761d2ec3a 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -629,44 +629,32 @@ vshCmdGrpSearch(const char *grpname)
 }
 
 bool
-vshCmdGrpHelp(vshControl *ctl, const char *grpname)
+vshCmdGrpHelp(vshControl *ctl, const vshCmdGrp *grp)
 {
-const vshCmdGrp *grp = vshCmdGrpSearch(grpname);
 const vshCmdDef *cmd = NULL;
 
-if (!grp) {
-vshError(ctl, _("command group '%s' doesn't exist"), grpname);
-return false;
-} else {
-vshPrint(ctl, _(" %s (help keyword '%s'):\n"), grp->name,
- grp->keyword);
+vshPrint(ctl, _(" %s (help keyword '%s'):\n"), grp->name,
+ grp->keyword);
 
-for (cmd = grp->commands; cmd->name; cmd++) {
-if (cmd->flags & VSH_CMD_FLAG_ALIAS)
-continue;
-vshPrint(ctl, "%-30s %s\n", cmd->name,
- _(vshCmddefGetInfo(cmd, "help")));
-}
+for (cmd = grp->commands; cmd->name; cmd++) {
+if (cmd->flags & VSH_CMD_FLAG_ALIAS)
+continue;
+vshPrint(ctl, "%-30s %s\n", cmd->name,
+ _(vshCmddefGetInfo(cmd, "help")));
 }
 
 return true;
 }
 
 bool
-vshCmddefHelp(vshControl *ctl, const char *cmdname)
+vshCmddefHelp(vshControl *ctl, const vshCmdDef *def)
 {
-const vshCmdDef *def = vshCmddefSearch(cmdname);
 const char *desc = NULL;
 char buf[256];
 uint64_t opts_need_arg;
 uint64_t opts_required;
 bool shortopt = false; /* true if 'arg' works instead of '--opt arg' */
 
-if (!def) {
-vshError(ctl, _("command '%s' doesn't exist"), cmdname);
-return false;
-}
-
 if (vshCmddefOptParse(def, _need_arg, _required)) {
 vshError(ctl, _("internal error: bad options in command: '%s'"),
  def->name);
@@ -3181,12 +3169,11 @@ const vshCmdInfo info_help[] = {
 bool
 cmdHelp(vshControl *ctl, const vshCmd *cmd)
 {
+const vshCmdDef *def = NULL;
+const vshCmdGrp *grp = NULL;
 const char *name = NULL;
 
 if (vshCommandOptStringQuiet(ctl, cmd, "command", ) <= 0) {
-const vshCmdGrp *grp;
-const vshCmdDef *def;
-
 vshPrint(ctl, "%s", _("Grouped commands:\n\n"));
 
 for (grp = cmdGroups; grp->name; grp++) {
@@ -3206,10 +3193,10 @@ cmdHelp(vshControl *ctl, const vshCmd *cmd)
 return true;
 }
 
-if (vshCmddefSearch(name)) {
-return vshCmddefHelp(ctl, name);
-} else if (vshCmdGrpSearch(name)) {
-return vshCmdGrpHelp(ctl, name);
+if ((def = vshCmddefSearch(name))) {
+return vshCmddefHelp(ctl, def);
+} else if ((grp = vshCmdGrpSearch(name))) {
+return vshCmdGrpHelp(ctl, grp);
 } else {
 vshError(ctl, _("command or command group '%s' doesn't exist"), name);
 return false;
diff --git a/tools/vsh.h b/tools/vsh.h
index 6894700d9..694476471 100644
--- a/tools/vsh.h
+++ b/tools/vsh.h
@@ -257,9 +257,9 @@ void vshCloseLogFile(vshControl *ctl);
 
 const char *vshCmddefGetInfo(const vshCmdDef *cmd, const char *info);
 const vshCmdDef *vshCmddefSearch(const char *cmdname);
-bool vshCmddefHelp(vshControl *ctl, const char *name);
+bool vshCmddefHelp(vshControl *ctl, const vshCmdDef *def);
 const vshCmdGrp *vshCmdGrpSearch(const char *grpname);
-bool vshCmdGrpHelp(vshControl *ctl, const char *name);
+bool vshCmdGrpHelp(vshControl *ctl, const vshCmdGrp *grp);
 
 int vshCommandOptInt(vshControl *ctl, const vshCmd *cmd,
  const char *name, int *value)
-- 
2.13.6

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

[libvirt] [PATCH 0/2] vsh: Print help for the aliased cmd rather than the alias itself

[Erik Skultety]

The alias doesn't have any definition rather than a reference for the aliased
command. Therefore when printing help, we have to print help for the aliased
command, otherwise our faith is going to be met with SIGSEGV.

Erik Skultety (2):
  vsh: Drop redundant definition searches from vshCmd{def,Grp}Help
  vsh: Cmd aliases lookups should return results for the aliased command

 tools/vsh.c | 45 +
 tools/vsh.h |  4 ++--
 2 files changed, 19 insertions(+), 30 deletions(-)

--
2.13.6

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

Re: [libvirt] [PATCH v3] docs: formatdomain: Document the CPU feature 'name' attribute

[Jiri Denemark]

On Thu, Jan 25, 2018 at 10:09:01 +0100, Kashyap Chamarthy wrote:
> Currently, the CPU feature 'name' XML attribute, as in:
> 
> [...]
> 
>   IvyBridge
>   Intel
>   
> 
> [...]
> 
> isn't explicitly documented in formatdomain.html.
> 
> Document it now.
> 
> Signed-off-by: Kashyap Chamarthy 
> ---
> v3:
>  - Wrap text in paragraph; fix phrasing [John Ferlan]
> v2:
>  - Remove redundant line [Eduardo Habkost]
> 
>  docs/formatdomain.html.in | 15 +++
>  1 file changed, 15 insertions(+)

ACK

Jirka

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

Re: [libvirt] [PATCH] Migration: Preserve the failed job in case migration job is terminated beyond the perform phase.

[Jiri Denemark]

On Thu, Jan 25, 2018 at 19:51:23 +0530, Prerna Saxena wrote:
> In case of non-p2p migration, in case libvirt client gets disconnected from 
> source libvirt
> after PERFORM phase is over, the daemon just resets the current migration job.
> However, the VM could be left paused on both source and destination in such 
> case. In case
> the client reconnects and queries migration status, the job has been blanked 
> out from source libvirt,
> and this reconnected client has no clear way of figuring out if an unclean 
> migration had previously
> been aborted.

The virDomainGetState API should return VIR_DOMAIN_PAUSED with
VIR_DOMAIN_PAUSED_MIGRATION reason. Is this not enough?

> This patch calls out a "potentially" incomplete migration as a failed
> job, so that a client may

As you say it's potentially incomplete, so marking it as failed is not
completely correct. It's a split brain when the source cannot
distinguish whether the migration was successful or not.

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e8e0313..7c60d17 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4564,6 +4564,22 @@ qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver, 
> virDomainObjPtr obj)
>  qemuDomainObjSaveJob(driver, obj);
>  }
>  
> +
> +void
> +qemuDomainObjFailAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
> +{
> +qemuDomainObjPrivatePtr priv = obj->privateData;
> +VIR_FREE(priv->job.completed);
> +if (VIR_ALLOC(priv->job.completed) == 0) {
> +priv->job.current->type = VIR_DOMAIN_JOB_FAILED;
> +priv->job.completed = priv->job.current;

This will just leak the memory allocated for priv->job.completed by
overwriting the pointer to the one from priv->job.current, ...

> +} else {
> +VIR_WARN("Unable to allocate job.completed for VM %s", 
> obj->def->name);
> +}
> +qemuDomainObjResetAsyncJob(priv);

which will point to a freed memory after this call.

> +qemuDomainObjEndJob(driver, obj);

And while this is almost certainly (I didn't really check though) not
something you should call from a close callback, you don't save the
changes to the status XML so once libvirtd restarts, it will think the
domain is still being migrated.

Jirka

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

Re: [libvirt] [PATCH 3/5] conf: Remove obsolete feature

[Daniel P . Berrangé]

On Tue, Jan 23, 2018 at 04:45:02PM +0100, Andrea Bolognani wrote:
> Now that the  feature has been implemented, we
> can get rid of the original  feature and thus ensure all
> pSeries features are grouped together in the same sub-element.
> 
> Compatibility code is provided so that existing guests can be
> parsed and migrated successfully despite the change.

This is still not ok - we're breaking any application which is
reading the XML, because the  flag it used to have is now
gone.

I'm afraid we cannot change this and must keep the original
 flag as is.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 2/5] conf: Add pSeries features

[Daniel P . Berrangé]

On Tue, Jan 23, 2018 at 04:45:01PM +0100, Andrea Bolognani wrote:
> We're going to introduce a number of optional, pSeries-specific
> features, so we want to have a dedicated sub-element to avoid
> cluttering the  element.

I don't think we want todo this as all. Pretty much *every* single
existing  element is architecture specific. Adding a nested
level just for pseries is making it different from existing practice
and I don't see any benefit to that.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] libxl: resume lock process after failed migration

[Michal Privoznik]

On 01/24/2018 10:43 PM, Jim Fehlig wrote:
> During migration, the lock process is paused in the perform phase
> but not resumed if there is a subsequent failure, leaving the locked
> resource unprotected.
> 
> The perform phase itself can fail, in which case the lock process
> should be resumed before returning from perform. The finish phase
> could also fail on the destination host, in which case the migration
> is canceled in the confirm phase and the VM is resumed. The lock
> process needs to be resumed there as well.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_migration.c | 12 
>  1 file changed, 12 insertions(+)

ACK

Michal

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

Re: [libvirt] [PATCH] cpu: Add support for al57 Intel features

[Jiri Denemark]

On Fri, Jan 19, 2018 at 22:28:39 +0800, Feng, Shaohe  wrote:
> On 2018年01月09日 18:40, Jiri Denemark wrote:
> > On Mon, Jan 08, 2018 at 19:06:43 +0800, Feng, Shaohe  wrote:
> >> On 2018年01月05日 02:52, John Ferlan wrote:
> >>> On 12/17/2017 06:02 PM, Shaohe Feng wrote:
>  We can start qemu with a "cpu,+la57" to set 57-bit vitrual address
>  space. So VM can be aware that it need to enable 5-level paging.
> 
>  Corresponding QEMU commits:
>    al57 6c7c3c21f95dd9af8a0691c0dd29b07247984122
>  ---
> src/cpu/cpu_map.xml | 3 +++
> 1 file changed, 3 insertions(+)
> 
> >>> I think if you go through history of cpu_map.xml changes you'll find
> >>> when a new feature is added there are tests added as well - this would
> >>> thus seemingly need a test adjustment as well.
> >> Thanks John.
> >> Will add tests.
> > Adding such test usually means you go to tests/cputestdata/ directory
> > and run "./cpu-gather.sh | ./cpu-parse.sh" which will produce several
> > data files. Then you add a new test case in tests/cputest.c for this new
> > CPU and regenerate the files with expected results. If you do this
> > before adding the new feature, the patch which will then add it will
> > nicely show the new feature gets added into the expected results of the
> > new CPU test.
> >
> > Don't forget to install cpuid tool and make sure you have the latest
> > upstream qemu installed in the system.
> >
> > Jirka
> 
> Hi Jirka,
> 
> I went through the tests/cputestdata/ directory and found it’s for 
> defining features for different CPU models.
> 
> However, the la57 feature is not included in any current CPUs. So I 
> think we can get waived for the tests changes?

Hmm, I expected you had access to such host for testing...

Anyway, ACK to the patch then with the following diff squashed in
(apparently you didn't run make check):

diff --git a/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml 
b/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml
index 7ff998907d..6c2bbac190 100644
--- a/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_2.9.0-tcg.x86_64.xml
@@ -43,6 +43,7 @@
   
   
   
+  
   
   
   

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

Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute

[Eduardo Habkost]

On Thu, Jan 25, 2018 at 09:17:22AM -0500, John Ferlan wrote:
> [...]
> >>
> >>> +name attribute. For example, to explicitly specify
> >>
> >> s/specify/require
> > 
> > I used the verb 'specify' to indicate that there is an _action_ to be
> > taken.  To my non-native ears: "to explicitly require" sounds slightly
> > odd when asking to take an action.
> > 
> > But I'll defer to your native tounge intuition.
> > 
> 
> FWIW: I noted require because the generated XML in the example is:
> 
>  
> 
> I'm OK with the change from v3, but the XML is what I was keying off.
> Essentially the line (to me) says, this domain requires a CPU that is
> required to have the 'pcid' feature. Perhaps just being too literal though.

IMO, "requiring a CPU" is not the important part.  The most
important consequence of  is changing what exactly the
guest will see on the virtual CPU.  On most cases, this requires
a host CPU that provides the feature, but it's just a
consequence of how it works, not the main goal.

-- 
Eduardo

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

Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute

[Kashyap Chamarthy]

On Thu, Jan 25, 2018 at 09:17:22AM -0500, John Ferlan wrote:
> [...]
> 
> >>
> >>> +name attribute. For example, to explicitly specify
> >>
> >> s/specify/require
> > 
> > I used the verb 'specify' to indicate that there is an _action_ to be
> > taken.  To my non-native ears: "to explicitly require" sounds slightly
> > odd when asking to take an action.
> > 
> > But I'll defer to your native tounge intuition.
> > 
> 
> FWIW: I noted require because the generated XML in the example is:
> 
>  
> 
> I'm OK with the change from v3, but the XML is what I was keying off.
> Essentially the line (to me) says, this domain requires a CPU that is
> required to have the 'pcid' feature. Perhaps just being too literal though.

Ah, I see where you're coming from now.  

If you're merging v3, feel free to tweak it.

Thanks.

[...]

-- 
/kashyap

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

Re: [libvirt] [PATCH v2 1/2] conf: add support for setting OEM strings SMBIOS data fields

[John Ferlan]

On 01/25/2018 09:26 AM, Daniel P. Berrangé wrote:
> On Thu, Jan 25, 2018 at 09:03:38AM -0500, John Ferlan wrote:
>>
>>
>> On 01/17/2018 12:37 PM, Daniel P. Berrange wrote:
>>> The OEM strings table in SMBIOS allows the vendor to pass arbitrary
>>> strings into the guest OS. This can be used as a way to pass data to an
>>> application like cloud-init, or potentially as an alternative to the
>>> kernel command line for OS installers where you can't modify the install
>>> ISO image to change the kernel args.
>>>
>>> As an example, consider if cloud-init and anaconda supported OEM strings
>>> you could use something like
>>>
>>> 
>>>   cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/
>>>   
>>> anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os
>>> 
>>>
>>> use of a application specific prefix as illustrated above is
>>> recommended, but not mandated, so that an app can reliably identify
>>> which of the many OEM strings are targetted at it.
>>>
>>> Signed-off-by: Daniel P. Berrange 
>>> ---
>>>  docs/formatdomain.html.in | 13 
>>>  docs/schemas/domaincommon.rng |  9 +
>>>  src/conf/domain_conf.c| 47 
>>> +++
>>>  src/util/virsysinfo.c | 33 ++
>>>  src/util/virsysinfo.h | 10 +
>>>  5 files changed, 112 insertions(+)
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index d272cc1ba6..6af2d26209 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -411,6 +411,10 @@
>>>  entry name='version'0B98401 Pro/entry
>>>  entry name='serial'W1KS427111E/entry
>>>/baseBoard
>>> +  oemStrings
>>> +entrymyappname:some arbitrary data/entry
>>> +entryotherappname:more arbitrary data/entry
>>> +  /oemStrings
>>>  /sysinfo
>>>  ...
>>>  
>>> @@ -498,6 +502,15 @@
>>>  validation and date format checking, all values 
>>> are
>>>  passed as strings to the hypervisor driver.
>>>
>>> +  oemStrings
>>> +  
>>> +This is block 11 of SMBIOS. This element should appear once and
>>> +can have multiple entry child elements, each 
>>> providing
>>> +arbitrary string data. There are no restrictions on what data 
>>> can
>>> +be provided in the entries, however, if the data is intended 
>>> to be
>>
>> s/, however/; however
> 
> Using a ; instead of , before "however" is rather wierd / unusual.
> 
> 
> Regards,
> Daniel
> 

Strange - that's the way I've been taught when joining compound
sentences. Even Google I think agrees with usage of the semi-colon in
this case. Consider the following:

If this, however, was some other way to describe the usage, then using a
comma would be right; however, you're joining two sentences and using it
a compound sentence.

In the long run, I don't really care that much - I'm not an
english/grammar major ;-)

John

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

Re: [libvirt] [PATCH v2 1/2] conf: add support for setting OEM strings SMBIOS data fields

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 09:03:38AM -0500, John Ferlan wrote:
> 
> 
> On 01/17/2018 12:37 PM, Daniel P. Berrange wrote:
> > The OEM strings table in SMBIOS allows the vendor to pass arbitrary
> > strings into the guest OS. This can be used as a way to pass data to an
> > application like cloud-init, or potentially as an alternative to the
> > kernel command line for OS installers where you can't modify the install
> > ISO image to change the kernel args.
> > 
> > As an example, consider if cloud-init and anaconda supported OEM strings
> > you could use something like
> > 
> > 
> >   cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/
> >   
> > anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os
> > 
> > 
> > use of a application specific prefix as illustrated above is
> > recommended, but not mandated, so that an app can reliably identify
> > which of the many OEM strings are targetted at it.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  docs/formatdomain.html.in | 13 
> >  docs/schemas/domaincommon.rng |  9 +
> >  src/conf/domain_conf.c| 47 
> > +++
> >  src/util/virsysinfo.c | 33 ++
> >  src/util/virsysinfo.h | 10 +
> >  5 files changed, 112 insertions(+)
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index d272cc1ba6..6af2d26209 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -411,6 +411,10 @@
> >  entry name='version'0B98401 Pro/entry
> >  entry name='serial'W1KS427111E/entry
> >/baseBoard
> > +  oemStrings
> > +entrymyappname:some arbitrary data/entry
> > +entryotherappname:more arbitrary data/entry
> > +  /oemStrings
> >  /sysinfo
> >  ...
> >  
> > @@ -498,6 +502,15 @@
> >  validation and date format checking, all values 
> > are
> >  passed as strings to the hypervisor driver.
> >
> > +  oemStrings
> > +  
> > +This is block 11 of SMBIOS. This element should appear once and
> > +can have multiple entry child elements, each 
> > providing
> > +arbitrary string data. There are no restrictions on what data 
> > can
> > +be provided in the entries, however, if the data is intended 
> > to be
> 
> s/, however/; however

Using a ; instead of , before "however" is rather wierd / unusual.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH 0/3] tools: Actually enable bash-completion

[Michal Privoznik]

Due to some naming convention the bash script for inactive
virsh/virt-admin wasn't loaded. Fix this.

At the same time, fix two small nits raised in the other patch set for
bash completion.

Michal Privoznik (3):
  virshDomainNameCompleter: Prune accepted flags
  virsh: Offer only persistent domains for autostart
  tools: Make symlinks to vsh bash-completion script

 libvirt.spec.in | 24 +++-
 tools/Makefile.am   |  7 ++-
 tools/virsh-completer.c | 13 +++--
 tools/virsh-domain.c|  2 +-
 4 files changed, 33 insertions(+), 13 deletions(-)

-- 
2.13.6

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

[libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

[Michal Privoznik]

Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
actually used in virsh code. Remove the unused ones.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-completer.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index e216d9076..e3b8234b4 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -45,18 +45,11 @@ virshDomainNameCompleter(vshControl *ctl,
 
 virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
   VIR_CONNECT_LIST_DOMAINS_INACTIVE |
+  VIR_CONNECT_LIST_DOMAINS_OTHER |
+  VIR_CONNECT_LIST_DOMAINS_PAUSED |
   VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
-  VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
   VIR_CONNECT_LIST_DOMAINS_RUNNING |
-  VIR_CONNECT_LIST_DOMAINS_PAUSED |
-  VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
-  VIR_CONNECT_LIST_DOMAINS_OTHER |
-  VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
-  VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
-  VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
-  VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
-  VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
-  VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
+  VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
   NULL);
 
 if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
-- 
2.13.6

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

[libvirt] [PATCH 3/3] tools: Make symlinks to vsh bash-completion script

[Michal Privoznik]

The bash-completion project documents that only those scripts
from $BASH_COMPLETIONS_DIR that share name with the current
command for which  was hit are loaded [1]. This means, that
vsh script we have there is not loaded. We have to create
symlinks for virsh and virt-admin.

At the same time, we have to create new RPM package because
virt-admin and client packages are independent. That means we
cannot place the vsh script in either of them. What we can do is
to have a different package that contains the completion script
and then virt-admin and client packages contain only the symlink
and require the bash-completion package.

1: https://github.com/scop/bash-completion#faq

Signed-off-by: Michal Privoznik 
---
 libvirt.spec.in   | 24 +++-
 tools/Makefile.am |  7 ++-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f62d7d324..1879e1f8b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1017,6 +1017,9 @@ Requires: gnutls-utils
 # Needed for probing the power management features of the host.
 Requires: pm-utils
 %endif
+%if %{with_bash_completion}
+Requires: %{name}-bash-completion = %{version}-%{release}
+%endif
 
 %description client
 The client binaries needed to access the virtualization
@@ -1041,10 +1044,22 @@ Summary: Set of tools to control libvirt daemon
 Group: Development/Libraries
 Requires: %{name}-libs = %{version}-%{release}
 Requires: readline
+%if %{with_bash_completion}
+Requires: %{name}-bash-completion = %{version}-%{release}
+%endif
 
 %description admin
 The client side utilities to control the libvirt daemon.
 
+%if %{with_bash_completion}
+%package bash-completion
+Summary: Bash completion script
+Group: Development/Libraries
+
+%description bash-completion
+Bash completion script stub.
+%endif
+
 %if %{with_wireshark}
 %package wireshark
 Summary: Wireshark dissector plugin for libvirt RPC transactions
@@ -2059,7 +2074,7 @@ exit 0
 %{_datadir}/systemtap/tapset/libvirt_functions.stp
 
 %if %{with_bash_completion}
-%{_datadir}/bash-completion/completions/vsh
+%{_datadir}/bash-completion/completions/virsh
 %endif
 
 
@@ -2111,7 +2126,14 @@ exit 0
 %files admin
 %{_mandir}/man1/virt-admin.1*
 %{_bindir}/virt-admin
+%if %{with_bash_completion}
+%{_datadir}/bash-completion/completions/virt-admin
+%endif
 
+%if %{with_bash_completion}
+%files bash-completion
+%{_datadir}/bash-completion/completions/vsh
+%endif
 
 %if %{with_wireshark}
 %files wireshark
diff --git a/tools/Makefile.am b/tools/Makefile.am
index e9597cdb4..e173f5634 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -424,9 +424,14 @@ install-bash-completion:
$(MKDIR_P) "$(DESTDIR)$(BASH_COMPLETIONS_DIR)"
$(INSTALL_SCRIPT) $(srcdir)/bash-completion/vsh \
"$(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh"
+   ( cd $(DESTDIR)$(BASH_COMPLETIONS_DIR) && \
+   $(LN_S) vsh virsh && \
+   $(LN_S) vsh virt-admin )
 
 uninstall-bash-completion:
-   rm -f $(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh
+   rm -f $(DESTDIR)$(BASH_COMPLETIONS_DIR)/vsh \
+   $(DESTDIR)$(BASH_COMPLETIONS_DIR)/virsh \
+   $(DESTDIR)$(BASH_COMPLETIONS_DIR)/virt-admin
rmdir $(DESTDIR)$(BASH_COMPLETIONS_DIR) ||:
 else ! WITH_BASH_COMPLETION
 install-bash-completion:
-- 
2.13.6

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

[libvirt] [PATCH 2/3] virsh: Offer only persistent domains for autostart

[Michal Privoznik]

The 'autostart' command accepts only persistent domains. Make the
completer return only those.

Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 0f329d6d7..5a0e0c1b2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1037,7 +1037,7 @@ static const vshCmdInfo info_autostart[] = {
 };
 
 static const vshCmdOptDef opts_autostart[] = {
-VIRSH_COMMON_OPT_DOMAIN_FULL(0),
+VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_PERSISTENT),
 {.name = "disable",
  .type = VSH_OT_BOOL,
  .help = N_("disable autostarting")
-- 
2.13.6

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

[libvirt] [PATCH] Migration: Preserve the failed job in case migration job is terminated beyond the perform phase.

[Prerna Saxena]

In case of non-p2p migration, in case libvirt client gets disconnected from 
source libvirt
after PERFORM phase is over, the daemon just resets the current migration job.
However, the VM could be left paused on both source and destination in such 
case. In case
the client reconnects and queries migration status, the job has been blanked 
out from source libvirt,
and this reconnected client has no clear way of figuring out if an unclean 
migration had previously
been aborted.

This patch calls out a "potentially" incomplete migration as a failed job, so 
that a client may
be able to watch previously failed jobs for this VM and take corrective actions 
as needed.

Signed-off-by: Prerna Saxena 
---
 src/qemu/qemu_domain.c|   16 
 src/qemu/qemu_domain.h|2 ++
 src/qemu/qemu_migration.c |4 ++--
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e8e0313..7c60d17 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4564,6 +4564,22 @@ qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver, 
virDomainObjPtr obj)
 qemuDomainObjSaveJob(driver, obj);
 }
 
+
+void
+qemuDomainObjFailAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
+{
+qemuDomainObjPrivatePtr priv = obj->privateData;
+VIR_FREE(priv->job.completed);
+if (VIR_ALLOC(priv->job.completed) == 0) {
+priv->job.current->type = VIR_DOMAIN_JOB_FAILED;
+priv->job.completed = priv->job.current;
+} else {
+VIR_WARN("Unable to allocate job.completed for VM %s", obj->def->name);
+}
+qemuDomainObjResetAsyncJob(priv);
+qemuDomainObjEndJob(driver, obj);
+}
+
 void
 qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj)
 {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index c33af36..6465603 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -497,6 +497,8 @@ void qemuDomainObjRestoreJob(virDomainObjPtr obj,
  struct qemuDomainJobObj *job);
 void qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver,
   virDomainObjPtr obj);
+void qemuDomainObjFailAsyncJob(virQEMUDriverPtr driver,
+  virDomainObjPtr obj);
 void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj);
 
 qemuMonitorPtr qemuDomainGetMonitor(virDomainObjPtr vm)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 69eb231..fd8950e 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1911,8 +1911,8 @@ qemuMigrationCleanup(virDomainObjPtr vm,
 VIR_WARN("Migration of domain %s finished but we don't know if the"
  " domain was successfully started on destination or not",
  vm->def->name);
-/* clear the job and let higher levels decide what to do */
-qemuDomainObjDiscardAsyncJob(driver, vm);
+/* clearly "fail" the job and let higher levels decide what to do */
+qemuDomainObjFailAsyncJob(driver, vm);
 break;
 
 case QEMU_MIGRATION_PHASE_PERFORM3:
-- 
1.7.1

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

Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute

[John Ferlan]

[...]

>>
>>> +name attribute. For example, to explicitly specify
>>
>> s/specify/require
> 
> I used the verb 'specify' to indicate that there is an _action_ to be
> taken.  To my non-native ears: "to explicitly require" sounds slightly
> odd when asking to take an action.
> 
> But I'll defer to your native tounge intuition.
> 

FWIW: I noted require because the generated XML in the example is:

 

I'm OK with the change from v3, but the XML is what I was keying off.
Essentially the line (to me) says, this domain requires a CPU that is
required to have the 'pcid' feature. Perhaps just being too literal though.

John

>> Thoughts?  I can make the adjustment before pushing if desired.
> 
> Thanks for the review.  Sending a v3; feel free to adjust it as you see
> fit.
> 

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

Re: [libvirt] [PATCH v2 2/2] qemu: add support for generating SMBIOS OEM strings command line

[John Ferlan]

On 01/17/2018 12:37 PM, Daniel P. Berrange wrote:
> This wires up the previously added OEM strings XML schema to be able to
> generate comamnd line args for QEMU. This requires QEMU >= 2.12 release
> containing this patch:
> 
>   commit 2d6dcbf93fb01b4a7f45a93d276d4d74b16392dd
>   Author: Daniel P. Berrange 
>   Date:   Sat Oct 28 21:51:36 2017 +0100
> 
> smbios: support setting OEM strings table
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_command.c | 28 
>  tests/qemuxml2argvdata/smbios.args  |  2 ++
>  tests/qemuxml2argvdata/smbios.xml   |  5 +
>  tests/qemuxml2xmloutdata/smbios.xml |  5 +
>  4 files changed, 40 insertions(+)
> 

Would a QEMU < 2.12 cause a failure if the 'type=11' OEM strings were
provided or do we need to add a capability in virQEMUCapsInitQMPMonitor
based on version?

What's here looks good and the provisional

Reviewed-by: John Ferlan 

if we don't need the capability. If we need it, then probably need to
update the patch.

John


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

Re: [libvirt] [PATCH v2 2/2] qemu: add support for generating SMBIOS OEM strings command line

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 09:04:05AM -0500, John Ferlan wrote:
> 
> 
> On 01/17/2018 12:37 PM, Daniel P. Berrange wrote:
> > This wires up the previously added OEM strings XML schema to be able to
> > generate comamnd line args for QEMU. This requires QEMU >= 2.12 release
> > containing this patch:
> > 
> >   commit 2d6dcbf93fb01b4a7f45a93d276d4d74b16392dd
> >   Author: Daniel P. Berrange 
> >   Date:   Sat Oct 28 21:51:36 2017 +0100
> > 
> > smbios: support setting OEM strings table
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  src/qemu/qemu_command.c | 28 
> >  tests/qemuxml2argvdata/smbios.args  |  2 ++
> >  tests/qemuxml2argvdata/smbios.xml   |  5 +
> >  tests/qemuxml2xmloutdata/smbios.xml |  5 +
> >  4 files changed, 40 insertions(+)
> > 
> 
> Would a QEMU < 2.12 cause a failure if the 'type=11' OEM strings were
> provided or do we need to add a capability in virQEMUCapsInitQMPMonitor
> based on version?

Yes it will

$ qemu-system-x86_64 -smbios type=11
qemu-system-x86_64: -smbios type=11: Don't know how to build fields for SMBIOS 
type 11


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 1/2] conf: add support for setting OEM strings SMBIOS data fields

[John Ferlan]

On 01/17/2018 12:37 PM, Daniel P. Berrange wrote:
> The OEM strings table in SMBIOS allows the vendor to pass arbitrary
> strings into the guest OS. This can be used as a way to pass data to an
> application like cloud-init, or potentially as an alternative to the
> kernel command line for OS installers where you can't modify the install
> ISO image to change the kernel args.
> 
> As an example, consider if cloud-init and anaconda supported OEM strings
> you could use something like
> 
> 
>   cloud-init:ds=nocloud-net;s=http://10.10.0.1:8000/
>   
> anaconda:method=http://dl.fedoraproject.org/pub/fedora/linux/releases/25/x86_64/os
> 
> 
> use of a application specific prefix as illustrated above is
> recommended, but not mandated, so that an app can reliably identify
> which of the many OEM strings are targetted at it.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  docs/formatdomain.html.in | 13 
>  docs/schemas/domaincommon.rng |  9 +
>  src/conf/domain_conf.c| 47 
> +++
>  src/util/virsysinfo.c | 33 ++
>  src/util/virsysinfo.h | 10 +
>  5 files changed, 112 insertions(+)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d272cc1ba6..6af2d26209 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -411,6 +411,10 @@
>  entry name='version'0B98401 Pro/entry
>  entry name='serial'W1KS427111E/entry
>/baseBoard
> +  oemStrings
> +entrymyappname:some arbitrary data/entry
> +entryotherappname:more arbitrary data/entry
> +  /oemStrings
>  /sysinfo
>  ...
>  
> @@ -498,6 +502,15 @@
>  validation and date format checking, all values are
>  passed as strings to the hypervisor driver.
>
> +  oemStrings
> +  
> +This is block 11 of SMBIOS. This element should appear once and
> +can have multiple entry child elements, each 
> providing
> +arbitrary string data. There are no restrictions on what data can
> +be provided in the entries, however, if the data is intended to 
> be

s/, however/; however

> +consumed by an application in the guest, it is recommended to use
> +the application name as a prefix in the string.

Add the Since 4.1.0

> +  
>  
>
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f22c932f6c..a154b5a462 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4857,6 +4857,15 @@
>  
>
>  
> +
> +  
> +
> +  
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a1c25060f9..6c0ad1ed7d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14461,6 +14461,42 @@ virSysinfoBaseBoardParseXML(xmlXPathContextPtr ctxt,
>  return ret;
>  }
>  

We've been doing the 2 blank lines between functions more recently -
although it's not officially in the hacking doc.

> +static int
> +virSysinfoOEMStringsParseXML(xmlNodePtr node,

@node is not used in this context so the compiler tells me ;-)

> + xmlXPathContextPtr ctxt,
> + virSysinfoOEMStringsDefPtr *oem)
> +{
> +int ret = -1;
> +virSysinfoOEMStringsDefPtr def;
> +xmlNodePtr *strings = NULL;
> +int nstrings;
> +size_t i;
> +
> +nstrings = virXPathNodeSet("./entry", ctxt, );
> +if (nstrings < 0)
> +return -1;
> +if (nstrings == 0)
> +return 0;
> +
> +if (VIR_ALLOC(def) < 0)
> +goto cleanup;
> +
> +if (VIR_ALLOC_N(def->values, nstrings) < 0)
> +goto cleanup;
> +
> +def->nvalues = nstrings;
> +for (i = 0; i < nstrings; i++)
> +def->values[i] = virXMLNodeContentString(strings[i]);
> +
> +*oem = def;
> +def = NULL;
> +ret = 0;
> + cleanup:
> +VIR_FREE(strings);
> +virSysinfoOEMStringsDefFree(def);

Coverity notes @def is leaked... [1]...

> +return ret;
> +}
> +
>  static virSysinfoDefPtr
>  virSysinfoParseXML(xmlNodePtr node,
>xmlXPathContextPtr ctxt,
> @@ -14519,6 +14555,17 @@ virSysinfoParseXML(xmlNodePtr node,
>  if (virSysinfoBaseBoardParseXML(ctxt, >baseBoard, >nbaseBoard) 
> < 0)
>  goto error;
>  
> +/* Extract system related metadata */
> +if ((tmpnode = virXPathNode("./oemStrings[1]", ctxt)) != NULL) {
> +oldnode = ctxt->node;
> +ctxt->node = tmpnode;
> +if (virSysinfoOEMStringsParseXML(tmpnode, ctxt, >oemStrings) < 
> 0) {
> +ctxt->node = oldnode;
> +goto error;
> +}
> +ctxt->node = oldnode;
> +}
> +
>   cleanup:
>  

Re: [libvirt] [PATCH] vbox: fix SEGV during dumpxml of a serial port

[Ján Tomko]

On Thu, Jan 25, 2018 at 07:31:14AM -0500, John Ferlan wrote:



On 01/20/2018 09:52 PM, Laine Stump wrote:

commit 77a12987a48 changed the "virDomainChrSourceDef source" inside
virDomainChrDef to "virDomainChrSourceDefPtr source", and started
allocating source inside virDomainChrDefNew(), but vboxDumpSerial()
was allocating a virDomainChrDef with a simple VIR_ALLOC(), so source
was never initialized, leading to a SEGV any time a serial port was
present. The same problem was created in vboxDumpParallel().

This patch changes vboxDumpSerial() and vboxDumpParallel() to use
virDomainChrDefNew() instead of VIR_ALLOC(), and makes a cursory
attempt to "recover" from OOM afterwards (much of the vbox code was
written to assume success, e.g. by having functions return void. Since
I have no way to test a more sweeping change to this code, I chose to
just short-circuit the rest of the function if virDomainChrDefNew()
fails - this is at least one step better than the previous code, which
would otherwise end up trying to dereference a NULL def->serials[i]
and crash libvirtd.

This resolves: https://bugzilla.redhat.com/1536649
---
 src/vbox/vbox_common.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 33aefabe5..c5fa5f08b 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3887,8 +3887,19 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr data, 
IMachine *machine, PRUin

 /* Allocate memory for the serial ports which are enabled */
 if ((def->nserials > 0) && (VIR_ALLOC_N(def->serials, def->nserials) >= 
0)) {
-for (i = 0; i < def->nserials; i++)
-ignore_value(VIR_ALLOC(def->serials[i]));
+for (i = 0; i < def->nserials; i++) {
+   def->serials[i] = virDomainChrDefNew(NULL);
+   if (!def->serials[i]) {
+   /* there is no provision for returning an error
+* (although the libvirtd logs will show an OOM error),


If you cannot allocate the chardev, maybe you won't be able to allocate
the OOM error message either.


+* but we need to at least prevent dereferencing
+* def->serials[i] and later (including continuing in
+* this function), as it will otherwise cause a SEGV.
+*/


As much as I like this because it's the kind of verbiage I'd put there -
I think we really should just fix the function to cause the failure and
cause the caller to fail as well since it's the right thing to do.

These end up being the dumpxml implementation details and by ignoring
failures we're probably creating bad XML.



Agreed.

This kind of half-fix with half a screen worth of comments is IMO a
waste of time. You're just going to crash on another unhandled
allocation error.

If you want the easy way out, you have my ACK to just change of the
allocation function from VIR_ALLOC to virDomainChrDefNew.

Jan


There have been some other recent adjustments in this space as recently
as 3.10.  In commit 'c27f79a89' the code changed void vboxDumpIDEHDDs to
be int vboxDumpDisks and went to cleanup.


John


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

Re: [libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 01:33:56PM +0100, Peter Krempa wrote:
> On Thu, Jan 25, 2018 at 09:38:13 +, Daniel Berrange wrote:
> > The QEMU driver loadable module needs to be able to resolve all ELF
> > symbols it references against libvirt.so. Some of its symbols can only
> > be resolved against the storage_driver.so loadable module which creates
> > a hard dependancy between them. By moving the storage file backend
> > framework into the util directory, this gets included directly in the
> > libvirt.so library. The actual backend implementations are still done as
> > loadable modules, so this doesn't re-add deps on gluster libraries.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  po/POTFILES.in |   3 +-
> >  src/Makefile.am|   3 +-
> >  src/libvirt_private.syms   |  19 +
> >  src/qemu/qemu_domain.c |   1 -
> >  src/qemu/qemu_driver.c |   1 -
> >  src/security/virt-aa-helper.c  |   2 -
> >  src/storage/storage_backend_fs.c   |   3 +-
> >  src/storage/storage_backend_gluster.c  |   3 +-
> >  src/storage/storage_source.c   | 645 
> > -
> >  src/storage/storage_source.h   |  59 --
> >  src/util/virstoragefile.c  | 609 
> > ++-
> >  src/util/virstoragefile.h  |  32 +
> >  .../virstoragefilebackend.c}   |   4 +-
> >  .../virstoragefilebackend.h}   |   8 +-
> >  tests/virstoragetest.c |   1 -
> >  15 files changed, 669 insertions(+), 724 deletions(-)
> >  delete mode 100644 src/storage/storage_source.c
> >  delete mode 100644 src/storage/storage_source.h
> >  rename src/{storage/storage_source_backend.c => 
> > util/virstoragefilebackend.c} (96%)
> >  rename src/{storage/storage_source_backend.h => 
> > util/virstoragefilebackend.h} (94%)
> 
> 
> [...]
> 
> > diff --git a/src/storage/storage_source_backend.h 
> > b/src/util/virstoragefilebackend.h
> > similarity index 94%
> > rename from src/storage/storage_source_backend.h
> > rename to src/util/virstoragefilebackend.h
> > index 8288bebb1f..6cd51750ee 100644
> > --- a/src/storage/storage_source_backend.h
> > +++ b/src/util/virstoragefilebackend.h
> > @@ -1,5 +1,5 @@
> >  /*
> > - * storage_source_backend.h: internal storage source backend contract
> > + * virstoragefilebackend.h: internal storage source backend contract
> >   *
> >   * Copyright (C) 2007-2018 Red Hat, Inc.
> >   *
> > @@ -18,8 +18,8 @@
> >   * .
> >   */
> >  
> > -#ifndef __VIR_STORAGE_SOURCE_BACKEND_H__
> > -# define __VIR_STORAGE_SOURCE_BACKEND_H__
> > +#ifndef __VIR_STORAGE_FILE_BACKEND_H__
> > +# define __VIR_STORAGE_FILE_BACKEND_H__
> >  
> >  # include 
> >  
> > @@ -101,4 +101,4 @@ struct _virStorageFileBackend {
> >  
> >  int virStorageFileBackendRegister(virStorageFileBackendPtr backend);
> >  
> > -#endif /* __VIR_STORAGE_BACKEND_H__ */
> > +#endif /* __VIR_STORAGE_FILE_BACKEND_H__ */
> 
> You've fixed it here, so you can disregard my previous comment.

Oh yes, but I meant to go back  and fix the previous patch anyway just
for sanity.

> 
> ACK



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Peter Krempa]

On Thu, Jan 25, 2018 at 09:38:13 +, Daniel Berrange wrote:
> The QEMU driver loadable module needs to be able to resolve all ELF
> symbols it references against libvirt.so. Some of its symbols can only
> be resolved against the storage_driver.so loadable module which creates
> a hard dependancy between them. By moving the storage file backend
> framework into the util directory, this gets included directly in the
> libvirt.so library. The actual backend implementations are still done as
> loadable modules, so this doesn't re-add deps on gluster libraries.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  po/POTFILES.in |   3 +-
>  src/Makefile.am|   3 +-
>  src/libvirt_private.syms   |  19 +
>  src/qemu/qemu_domain.c |   1 -
>  src/qemu/qemu_driver.c |   1 -
>  src/security/virt-aa-helper.c  |   2 -
>  src/storage/storage_backend_fs.c   |   3 +-
>  src/storage/storage_backend_gluster.c  |   3 +-
>  src/storage/storage_source.c   | 645 
> -
>  src/storage/storage_source.h   |  59 --
>  src/util/virstoragefile.c  | 609 ++-
>  src/util/virstoragefile.h  |  32 +
>  .../virstoragefilebackend.c}   |   4 +-
>  .../virstoragefilebackend.h}   |   8 +-
>  tests/virstoragetest.c |   1 -
>  15 files changed, 669 insertions(+), 724 deletions(-)
>  delete mode 100644 src/storage/storage_source.c
>  delete mode 100644 src/storage/storage_source.h
>  rename src/{storage/storage_source_backend.c => 
> util/virstoragefilebackend.c} (96%)
>  rename src/{storage/storage_source_backend.h => 
> util/virstoragefilebackend.h} (94%)


[...]

> diff --git a/src/storage/storage_source_backend.h 
> b/src/util/virstoragefilebackend.h
> similarity index 94%
> rename from src/storage/storage_source_backend.h
> rename to src/util/virstoragefilebackend.h
> index 8288bebb1f..6cd51750ee 100644
> --- a/src/storage/storage_source_backend.h
> +++ b/src/util/virstoragefilebackend.h
> @@ -1,5 +1,5 @@
>  /*
> - * storage_source_backend.h: internal storage source backend contract
> + * virstoragefilebackend.h: internal storage source backend contract
>   *
>   * Copyright (C) 2007-2018 Red Hat, Inc.
>   *
> @@ -18,8 +18,8 @@
>   * .
>   */
>  
> -#ifndef __VIR_STORAGE_SOURCE_BACKEND_H__
> -# define __VIR_STORAGE_SOURCE_BACKEND_H__
> +#ifndef __VIR_STORAGE_FILE_BACKEND_H__
> +# define __VIR_STORAGE_FILE_BACKEND_H__
>  
>  # include 
>  
> @@ -101,4 +101,4 @@ struct _virStorageFileBackend {
>  
>  int virStorageFileBackendRegister(virStorageFileBackendPtr backend);
>  
> -#endif /* __VIR_STORAGE_BACKEND_H__ */
> +#endif /* __VIR_STORAGE_FILE_BACKEND_H__ */

You've fixed it here, so you can disregard my previous comment.

ACK


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

Re: [libvirt] [PATCH] vbox: fix SEGV during dumpxml of a serial port

[John Ferlan]

On 01/20/2018 09:52 PM, Laine Stump wrote:
> commit 77a12987a48 changed the "virDomainChrSourceDef source" inside
> virDomainChrDef to "virDomainChrSourceDefPtr source", and started
> allocating source inside virDomainChrDefNew(), but vboxDumpSerial()
> was allocating a virDomainChrDef with a simple VIR_ALLOC(), so source
> was never initialized, leading to a SEGV any time a serial port was
> present. The same problem was created in vboxDumpParallel().
> 
> This patch changes vboxDumpSerial() and vboxDumpParallel() to use
> virDomainChrDefNew() instead of VIR_ALLOC(), and makes a cursory
> attempt to "recover" from OOM afterwards (much of the vbox code was
> written to assume success, e.g. by having functions return void. Since
> I have no way to test a more sweeping change to this code, I chose to
> just short-circuit the rest of the function if virDomainChrDefNew()
> fails - this is at least one step better than the previous code, which
> would otherwise end up trying to dereference a NULL def->serials[i]
> and crash libvirtd.
> 
> This resolves: https://bugzilla.redhat.com/1536649
> ---
>  src/vbox/vbox_common.c | 30 ++
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 33aefabe5..c5fa5f08b 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -3887,8 +3887,19 @@ vboxDumpSerial(virDomainDefPtr def, vboxDriverPtr 
> data, IMachine *machine, PRUin
>  
>  /* Allocate memory for the serial ports which are enabled */
>  if ((def->nserials > 0) && (VIR_ALLOC_N(def->serials, def->nserials) >= 
> 0)) {
> -for (i = 0; i < def->nserials; i++)
> -ignore_value(VIR_ALLOC(def->serials[i]));
> +for (i = 0; i < def->nserials; i++) {
> +   def->serials[i] = virDomainChrDefNew(NULL);
> +   if (!def->serials[i]) {
> +   /* there is no provision for returning an error
> +* (although the libvirtd logs will show an OOM error),
> +* but we need to at least prevent dereferencing
> +* def->serials[i] and later (including continuing in
> +* this function), as it will otherwise cause a SEGV.
> +*/

As much as I like this because it's the kind of verbiage I'd put there -
I think we really should just fix the function to cause the failure and
cause the caller to fail as well since it's the right thing to do.

These end up being the dumpxml implementation details and by ignoring
failures we're probably creating bad XML.

There have been some other recent adjustments in this space as recently
as 3.10.  In commit 'c27f79a89' the code changed void vboxDumpIDEHDDs to
be int vboxDumpDisks and went to cleanup.


John

> +   def->nserials = i;
> +   return;
> +   }
> +}
>  }
>  
>  /* Now get the details about the serial ports here */
> @@ -3975,8 +3986,19 @@ vboxDumpParallel(virDomainDefPtr def, vboxDriverPtr 
> data, IMachine *machine, PRU
>  
>  /* Allocate memory for the parallel ports which are enabled */
>  if ((def->nparallels > 0) && (VIR_ALLOC_N(def->parallels, 
> def->nparallels) >= 0)) {
> -for (i = 0; i < def->nparallels; i++)
> -ignore_value(VIR_ALLOC(def->parallels[i]));
> +for (i = 0; i < def->nparallels; i++) {
> +   def->parallels[i] = virDomainChrDefNew(NULL);
> +   if (!def->parallels[i]) {
> +   /* there is no provision for returning an error
> +* (although the libvirtd logs will show an OOM error),
> +* but we need to at least prevent dereferencing
> +* def->parallels[i] and later (including continuing in
> +* this function), as it will otherwise cause a SEGV.
> +*/
> +   def->nparallels = i;
> +   return;
> +   }
> +}
>  }
>  
>  /* Now get the details about the parallel ports here */
> 

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

Re: [libvirt] [PATCH 01/14] storage: extract storage file backend from main storage driver backend

[Peter Krempa]

On Thu, Jan 25, 2018 at 09:38:12 +, Daniel Berrange wrote:
> The storage driver backends are serving the public storage pools API,
> while the storage file backends are serving the internal QEMU driver and
> / or libvirt utility code.
> 
> To prep for moving this storage file backend framework into the utility
> code, split out the backend definitions.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  po/POTFILES.in|   1 +
>  src/Makefile.am   |   1 +
>  src/storage/storage_backend.c |  66 -
>  src/storage/storage_backend.h |  75 ---
>  src/storage/storage_backend_fs.c  |   7 ++-
>  src/storage/storage_backend_gluster.c |   3 +-
>  src/storage/storage_source.c  |   2 +-
>  src/storage/storage_source_backend.c  | 108 
> ++
>  src/storage/storage_source_backend.h  | 104 
>  9 files changed, 221 insertions(+), 146 deletions(-)
>  create mode 100644 src/storage/storage_source_backend.c
>  create mode 100644 src/storage/storage_source_backend.h
> 

[...]

> diff --git a/src/storage/storage_source_backend.h 
> b/src/storage/storage_source_backend.h
> new file mode 100644
> index 00..8288bebb1f
> --- /dev/null
> +++ b/src/storage/storage_source_backend.h
> @@ -0,0 +1,104 @@
> +/*
> + * storage_source_backend.h: internal storage source backend contract
> + *
> + * Copyright (C) 2007-2018 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + */
> +
> +#ifndef __VIR_STORAGE_SOURCE_BACKEND_H__
> +# define __VIR_STORAGE_SOURCE_BACKEND_H__


[...]

> +
> +
> +int virStorageFileBackendRegister(virStorageFileBackendPtr backend);
> +
> +#endif /* __VIR_STORAGE_BACKEND_H__ */

The comment is not matching the previously defined macro.

ACK with it fixed.


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

Re: [libvirt] [PATCH 05/10] resctrl: Add functions to work with resctrl allocations

[Pavel Hrdina]

On Wed, Jan 24, 2018 at 10:57:05PM +0100, Martin Kletzander wrote:
> On Wed, Jan 24, 2018 at 02:51:46PM +0100, Pavel Hrdina wrote:
> > On Tue, Jan 23, 2018 at 07:05:14PM +0100, Martin Kletzander wrote:
> > > With this commit we finally have a way to read and manipulate basic 
> > > resctrl
> > > settings.  Locking is done only on exposed functions that read/write 
> > > from/to
> > > resctrlfs.  Not in functions that are exposed in virresctrlpriv.h as 
> > > those are
> > > only supposed to be used from tests.
> > > 
> > > More information about how resctrl works:
> > > 
> > >   
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/x86/intel_rdt_ui.txt
> > > 
> > > Signed-off-by: Martin Kletzander 
> > > ---
> > >  src/Makefile.am   |2 +-
> > >  src/libvirt_private.syms  |   10 +
> > >  src/util/virresctrl.c | 1211 
> > > -
> > >  src/util/virresctrl.h |   49 ++
> > >  src/util/virresctrlpriv.h |   27 +
> > >  5 files changed, 1288 insertions(+), 11 deletions(-)
> > >  create mode 100644 src/util/virresctrlpriv.h

With the fixes included

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 11:54:42AM +0100, Peter Krempa wrote:
> On Thu, Jan 25, 2018 at 10:38:32 +, Daniel Berrange wrote:
> > On Thu, Jan 25, 2018 at 11:33:57AM +0100, Peter Krempa wrote:
> > > On Thu, Jan 25, 2018 at 09:38:13 +, Daniel Berrange wrote:
> > > > The QEMU driver loadable module needs to be able to resolve all ELF
> > > > symbols it references against libvirt.so. Some of its symbols can only
> > > > be resolved against the storage_driver.so loadable module which creates
> > > > a hard dependancy between them. By moving the storage file backend
> > > > framework into the util directory, this gets included directly in the
> > > > libvirt.so library. The actual backend implementations are still done as
> > > > loadable modules, so this doesn't re-add deps on gluster libraries.
> > > > 
> > > > Signed-off-by: Daniel P. Berrange 
> > > > ---
> > > 
> > > [...]
> > > 
> > > > diff --git a/src/storage/storage_source_backend.h 
> > > > b/src/util/virstoragefilebackend.h
> > > > similarity index 94%
> > > > rename from src/storage/storage_source_backend.h
> > > > rename to src/util/virstoragefilebackend.h
> > > > index 8288bebb1f..6cd51750ee 100644
> > > > --- a/src/storage/storage_source_backend.h
> > > > +++ b/src/util/virstoragefilebackend.h
> > > > @@ -1,5 +1,5 @@
> > > >  /*
> > > > - * storage_source_backend.h: internal storage source backend contract
> > > > + * virstoragefilebackend.h: internal storage source backend contracta
> > > 
> > > I was actually striving to move and rename all the stuff dealing with
> > > virStorageSource to be called with the appropriate prefix. This will
> > > also need splitting of src/util/virstoragefile.c, so this would be a
> > > regression in naming in my opinion.
> > 
> > All these APIs are currently called virStorageFile right now though,
> > so this is really fixing the inconsistency we already had between API
> > name and file name right now.  I agree though, it could be nice to
> > further rename the APIs to be called virStorageSource, and move
> > them into a virstoragesource.{c,h} file, but I think that's a separate
> > followup since its a pre-existing problem with virstoragefile.{c,h}. 
> 
> Hmm, yeah it's pre-existing for a long time and incidentally I don't
> know why I used the 'virStorageFile' prefix in this case.

Probably just copying the precedent I set when I first created the
virstoragefile.h file and then we all just add stuff until someone
decides it has got too messy :-)

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Peter Krempa]

On Thu, Jan 25, 2018 at 10:38:32 +, Daniel Berrange wrote:
> On Thu, Jan 25, 2018 at 11:33:57AM +0100, Peter Krempa wrote:
> > On Thu, Jan 25, 2018 at 09:38:13 +, Daniel Berrange wrote:
> > > The QEMU driver loadable module needs to be able to resolve all ELF
> > > symbols it references against libvirt.so. Some of its symbols can only
> > > be resolved against the storage_driver.so loadable module which creates
> > > a hard dependancy between them. By moving the storage file backend
> > > framework into the util directory, this gets included directly in the
> > > libvirt.so library. The actual backend implementations are still done as
> > > loadable modules, so this doesn't re-add deps on gluster libraries.
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > 
> > [...]
> > 
> > > diff --git a/src/storage/storage_source_backend.h 
> > > b/src/util/virstoragefilebackend.h
> > > similarity index 94%
> > > rename from src/storage/storage_source_backend.h
> > > rename to src/util/virstoragefilebackend.h
> > > index 8288bebb1f..6cd51750ee 100644
> > > --- a/src/storage/storage_source_backend.h
> > > +++ b/src/util/virstoragefilebackend.h
> > > @@ -1,5 +1,5 @@
> > >  /*
> > > - * storage_source_backend.h: internal storage source backend contract
> > > + * virstoragefilebackend.h: internal storage source backend contracta
> > 
> > I was actually striving to move and rename all the stuff dealing with
> > virStorageSource to be called with the appropriate prefix. This will
> > also need splitting of src/util/virstoragefile.c, so this would be a
> > regression in naming in my opinion.
> 
> All these APIs are currently called virStorageFile right now though,
> so this is really fixing the inconsistency we already had between API
> name and file name right now.  I agree though, it could be nice to
> further rename the APIs to be called virStorageSource, and move
> them into a virstoragesource.{c,h} file, but I think that's a separate
> followup since its a pre-existing problem with virstoragefile.{c,h}. 

Hmm, yeah it's pre-existing for a long time and incidentally I don't
know why I used the 'virStorageFile' prefix in this case.

You are right that at least for now it will be consistent.

I'll look again and provide proper review at least for the storage
driver part.


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

Re: [libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 11:33:57AM +0100, Peter Krempa wrote:
> On Thu, Jan 25, 2018 at 09:38:13 +, Daniel Berrange wrote:
> > The QEMU driver loadable module needs to be able to resolve all ELF
> > symbols it references against libvirt.so. Some of its symbols can only
> > be resolved against the storage_driver.so loadable module which creates
> > a hard dependancy between them. By moving the storage file backend
> > framework into the util directory, this gets included directly in the
> > libvirt.so library. The actual backend implementations are still done as
> > loadable modules, so this doesn't re-add deps on gluster libraries.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> 
> [...]
> 
> > diff --git a/src/storage/storage_source_backend.h 
> > b/src/util/virstoragefilebackend.h
> > similarity index 94%
> > rename from src/storage/storage_source_backend.h
> > rename to src/util/virstoragefilebackend.h
> > index 8288bebb1f..6cd51750ee 100644
> > --- a/src/storage/storage_source_backend.h
> > +++ b/src/util/virstoragefilebackend.h
> > @@ -1,5 +1,5 @@
> >  /*
> > - * storage_source_backend.h: internal storage source backend contract
> > + * virstoragefilebackend.h: internal storage source backend contracta
> 
> I was actually striving to move and rename all the stuff dealing with
> virStorageSource to be called with the appropriate prefix. This will
> also need splitting of src/util/virstoragefile.c, so this would be a
> regression in naming in my opinion.

All these APIs are currently called virStorageFile right now though,
so this is really fixing the inconsistency we already had between API
name and file name right now.  I agree though, it could be nice to
further rename the APIs to be called virStorageSource, and move
them into a virstoragesource.{c,h} file, but I think that's a separate
followup since its a pre-existing problem with virstoragefile.{c,h}. 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Peter Krempa]

On Thu, Jan 25, 2018 at 09:38:13 +, Daniel Berrange wrote:
> The QEMU driver loadable module needs to be able to resolve all ELF
> symbols it references against libvirt.so. Some of its symbols can only
> be resolved against the storage_driver.so loadable module which creates
> a hard dependancy between them. By moving the storage file backend
> framework into the util directory, this gets included directly in the
> libvirt.so library. The actual backend implementations are still done as
> loadable modules, so this doesn't re-add deps on gluster libraries.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

[...]

> diff --git a/src/storage/storage_source_backend.h 
> b/src/util/virstoragefilebackend.h
> similarity index 94%
> rename from src/storage/storage_source_backend.h
> rename to src/util/virstoragefilebackend.h
> index 8288bebb1f..6cd51750ee 100644
> --- a/src/storage/storage_source_backend.h
> +++ b/src/util/virstoragefilebackend.h
> @@ -1,5 +1,5 @@
>  /*
> - * storage_source_backend.h: internal storage source backend contract
> + * virstoragefilebackend.h: internal storage source backend contracta

I was actually striving to move and rename all the stuff dealing with
virStorageSource to be called with the appropriate prefix. This will
also need splitting of src/util/virstoragefile.c, so this would be a
regression in naming in my opinion.


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

Re: [libvirt] [PATCH hooks 1/1] Add check for Signed-off-by in commit messages

[Peter Krempa]

On Mon, Jan 22, 2018 at 13:57:07 +, Daniel Berrange wrote:
> On Mon, Jan 22, 2018 at 02:20:52PM +0100, Peter Krempa wrote:
> > On Mon, Jan 22, 2018 at 13:06:28 +, Daniel Berrange wrote:
> > > On Mon, Jan 22, 2018 at 01:20:12PM +0100, Peter Krempa wrote:
> > > > On Mon, Jan 22, 2018 at 12:05:19 +, Daniel Berrange wrote:
> > > > > This extends the update hook so that it enforces a requirement to 
> > > > > have a
> > > > > Signed-off-by line in every commit message. This can be optionally
> > > > > turned off in individual repos by setting the "hooks.allowmissingsob"
> > > > > git config variable.
> > > > > 
> > > > > Signed-off-by: Daniel P. Berrange 
> > > > > ---
> > > > >  update | 16 +++-
> > > > >  1 file changed, 15 insertions(+), 1 deletion(-)

[..]

> > The sign-off by itself (whithout cryptographic signature) is just
> > pointless. Validity with a cryptographic signature from drive-by
> > contributors can still be unproven, but at least you don't get
> > impersonation.
> 
> I think these are two different axis. The sob isn't trying to address the
> question of impersonation. It obviously has as a starting point that you
> accept the identity of the submitter to some degree. I accept that if you
> have cryptographically signed patches, that would give a stronger
> validation of identity, but there's never any absolutes. So not having
> a crypto signature doesn't make the sob invalid.

In that case basically nothing changes, since if we are going to use
this to be safe from licensing disputes, the reviewer/commiter still
needs to make sure that the code complies with our licensing. Asserting
the signoff changes nothing in that regard

> 
> > If everything is signed off, nothing really is.
> 
> I don't really see that.
> 
> > NACK still stands.
> 
> You are nacking something that you've accepted above will have no negative
> impact on your work, but has potentially significant upside to the project.
> That is very disappointing.

I think that by doing this we'll put too much false hope into the
"potentially significant upside". I just hope it will not bite us.

Anyone can assert, or sign-off anything [1].

Given the overwhelmingly positive approach to this retract my NACK, the
only thing that will change in general is that my commits will grow one
line.

I hope that I'm wrong with my pessimistic view.

Peter

[1] https://en.wikipedia.org/wiki/On_the_Internet,_nobody_knows_you're_a_dog


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

[libvirt] [PATCH 12/14] build: provide a AM_FLAGS_MOD for loadable modules

[Daniel P . Berrangé]

Dynamic loadable modules all need a common set of linker flags

  -module -avoid-version $(AM_LDFLAGS)

Bundle those up into a $(AM_LDFLAGS_MOD) to avoid repetition.

Signed-off-by: Daniel P. Berrangé 
---
 src/Makefile.am | 69 +++--
 1 file changed, 28 insertions(+), 41 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 492bbac22e..335b3a0c81 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -51,6 +51,7 @@ AM_LDFLAGS =  $(DRIVER_MODULES_LDFLAGS) \
$(CYGWIN_EXTRA_LDFLAGS) \
$(MINGW_EXTRA_LDFLAGS) \
$(NULL)
+AM_LDFLAGS_MOD = -module -avoid-version $(AM_LDFLAGS)
 
 POD2MAN = pod2man -c "Virtualization Support" -r "$(PACKAGE)-$(VERSION)"
 
@@ -1335,7 +1336,7 @@ libvirt_driver_xen_la_SOURCES =
 libvirt_driver_xen_la_LIBADD = libvirt_driver_xen_impl.la
 mod_LTLIBRARIES += libvirt_driver_xen.la
 libvirt_driver_xen_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_xen_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_xen_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_xen_impl_la_CFLAGS = \
$(XEN_CFLAGS) \
@@ -1381,7 +1382,7 @@ libvirt_driver_vbox_la_LIBADD = 
libvirt_driver_vbox_impl.la
 mod_LTLIBRARIES += \
libvirt_driver_vbox.la
 libvirt_driver_vbox_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_vbox_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_vbox_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_vbox_impl_la_CFLAGS = \
-I$(srcdir)/conf \
@@ -1410,7 +1411,7 @@ libvirt_driver_libxl_la_SOURCES =
 libvirt_driver_libxl_la_LIBADD = libvirt_driver_libxl_impl.la
 mod_LTLIBRARIES += libvirt_driver_libxl.la
 libvirt_driver_libxl_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_libxl_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_libxl_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_libxl_impl_la_CFLAGS = \
$(LIBXL_CFLAGS) \
@@ -1439,7 +1440,7 @@ libvirt_driver_qemu_la_SOURCES =
 libvirt_driver_qemu_la_LIBADD = libvirt_driver_qemu_impl.la
 mod_LTLIBRARIES += libvirt_driver_qemu.la
 libvirt_driver_qemu_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_qemu_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_qemu_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_qemu_impl_la_CFLAGS = \
$(GNUTLS_CFLAGS) \
@@ -1474,7 +1475,7 @@ libvirt_driver_lxc_la_SOURCES =
 libvirt_driver_lxc_la_LIBADD = libvirt_driver_lxc_impl.la
 mod_LTLIBRARIES += libvirt_driver_lxc.la
 libvirt_driver_lxc_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_lxc_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_lxc_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_lxc_impl_la_CFLAGS = \
$(LIBNL_CFLAGS) \
@@ -1511,7 +1512,7 @@ libvirt_driver_uml_la_SOURCES =
 libvirt_driver_uml_la_LIBADD = libvirt_driver_uml_impl.la
 mod_LTLIBRARIES += libvirt_driver_uml.la
 libvirt_driver_uml_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_uml_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_uml_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_uml_impl_la_CFLAGS = \
-I$(srcdir)/access \
@@ -1583,7 +1584,7 @@ libvirt_driver_vz_la_SOURCES =
 libvirt_driver_vz_la_LIBADD = libvirt_driver_vz_impl.la
 mod_LTLIBRARIES += libvirt_driver_vz.la
 libvirt_driver_vz_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_vz_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_vz_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 libvirt_driver_vz_impl_la_CFLAGS = \
-I$(srcdir)/conf \
-I$(srcdir)/access \
@@ -1599,7 +1600,7 @@ libvirt_driver_bhyve_la_SOURCES =
 libvirt_driver_bhyve_la_LIBADD = libvirt_driver_bhyve_impl.la
 mod_LTLIBRARIES += libvirt_driver_bhyve.la
 libvirt_driver_bhyve_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_bhyve_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_bhyve_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_bhyve_impl_la_CFLAGS = \
-I$(srcdir)/access \
@@ -1625,7 +1626,7 @@ libvirt_driver_network_la_LIBADD += libvirt.la 
../gnulib/lib/libgnu.la \
$(LIBNL_LIBS) \
$(DBUS_LIBS) \
$(NULL)
-libvirt_driver_network_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
+libvirt_driver_network_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 
 libvirt_driver_network_impl_la_CFLAGS = \
$(LIBNL_CFLAGS) \
@@ -1645,7 +1646,7 @@ libvirt_driver_interface_la_CFLAGS = \
-I$(srcdir)/access \
-I$(srcdir)/conf \
$(AM_CFLAGS) $(LIBNL_CFLAGS)
-libvirt_driver_interface_la_LDFLAGS = $(AM_LDFLAGS)
+libvirt_driver_interface_la_LDFLAGS = $(AM_LDFLAGS_MOD)
 libvirt_driver_interface_la_LIBADD =
 if WITH_NETCF
 libvirt_driver_interface_la_CFLAGS += $(NETCF_CFLAGS)
@@ -1656,7 +1657,6 @@ 

[libvirt] [PATCH 14/14] build: link libvirt_lxc against libvirt.so

[Daniel P . Berrangé]

Rather than static linking in various of the helper libraries to
libvirt_lxc, just link against the main libvirt.so. This is more memory
and time efficient because it will already be cached in memory and
sharable between processes.

Signed-off-by: Daniel P. Berrangé 
---
 src/Makefile.am  | 6 +-
 src/libvirt_private.syms | 2 ++
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 8fddafee51..1dc518fab9 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -3084,11 +3084,7 @@ libvirt_lxc_LDFLAGS = \
$(NULL)
 libvirt_lxc_LDADD = \
$(FUSE_LIBS) \
-   libvirt-net-rpc-server.la \
-   libvirt-net-rpc.la \
-   libvirt_security_manager.la \
-   libvirt_conf.la \
-   libvirt_util.la \
+   libvirt.la \
../gnulib/lib/libgnu.la
 if WITH_DTRACE_PROBES
 libvirt_lxc_LDADD += libvirt_probes.lo
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7ebd0f6066..a22bc80b06 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -347,6 +347,7 @@ virDomainDiskSetType;
 virDomainDiskTranslateSourcePool;
 virDomainFSDefFree;
 virDomainFSDefNew;
+virDomainFSDriverTypeToString;
 virDomainFSIndexByName;
 virDomainFSInsert;
 virDomainFSRemove;
@@ -476,6 +477,7 @@ virDomainObjGetOneDefState;
 virDomainObjGetPersistentDef;
 virDomainObjGetState;
 virDomainObjNew;
+virDomainObjParseFile;
 virDomainObjParseNode;
 virDomainObjRemoveTransientDef;
 virDomainObjSetDefTransient;
-- 
2.14.3

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

[libvirt] [PATCH 13/14] build: passing the "-z defs" linker flag to prevent undefined symbols

[Daniel P . Berrangé]

Undefined symbols are a bad thing in general because they can get
resolved in unexpected ways at runtime if multiple sources provide the
same symbol name. For example both glibc and libtirpc may provide XDR
symbols and we want to ensure that we resolve to libtirpc if that's what
we originally built against.

The toolchain maintainers thus strongly recommend that all applications
use the '-z defs' linker flag to prevent undefined symbols. This is
shortly becoming part of the default linker flags for RPMs. As an added
benefit this aligns Linux builds with Windows builds, where the linker
has never permitted undefined symbols.

Signed-off-by: Daniel P. Berrange 
---
 configure.ac   |  1 +
 daemon/Makefile.am |  3 +++
 m4/virt-linker-no-undefined.m4 | 32 
 src/Makefile.am| 36 
 tools/Makefile.am  |  1 +
 5 files changed, 57 insertions(+), 16 deletions(-)
 create mode 100644 m4/virt-linker-no-undefined.m4

diff --git a/configure.ac b/configure.ac
index 4cccf7f4de..7997ec5a14 100644
--- a/configure.ac
+++ b/configure.ac
@@ -237,6 +237,7 @@ LIBVIRT_COMPILE_WARNINGS
 LIBVIRT_COMPILE_PIE
 LIBVIRT_LINKER_RELRO
 LIBVIRT_LINKER_NO_INDIRECT
+LIBVIRT_LINKER_NO_UNDEFINED
 
 LIBVIRT_ARG_APPARMOR
 LIBVIRT_ARG_ATTR
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index f7519efe2f..e20b551be9 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -133,6 +133,7 @@ libvirtd_conf_la_LDFLAGS = \
$(PIE_LDFLAGS) \
$(COVERAGE_LDFLAGS) \
$(NO_INDIRECT_LDFLAGS) \
+   $(NO_UNDEFINED_LDFLAGS) \
$(NULL)
 libvirtd_conf_la_LIBADD = $(LIBXML_LIBS)
 
@@ -153,6 +154,7 @@ libvirtd_admin_la_LDFLAGS = \
$(RELRO_LDFLAGS) \
$(COVERAGE_LDFLAGS) \
$(NO_INDIRECT_LDFLAGS) \
+   $(NO_UNDEFINED_LDFLAGS) \
$(NULL)
 libvirtd_admin_la_LIBADD = \
../src/libvirt-admin.la
@@ -187,6 +189,7 @@ libvirtd_LDFLAGS = \
$(PIE_LDFLAGS) \
$(COVERAGE_LDFLAGS) \
$(NO_INDIRECT_LDFLAGS) \
+   $(NO_UNDEFINED_LDFLAGS) \
$(NULL)
 
 libvirtd_LDADD = \
diff --git a/m4/virt-linker-no-undefined.m4 b/m4/virt-linker-no-undefined.m4
new file mode 100644
index 00..532b0de212
--- /dev/null
+++ b/m4/virt-linker-no-undefined.m4
@@ -0,0 +1,32 @@
+dnl
+dnl Check for -z defs linker flag
+dnl
+dnl Copyright (C) 2013-2018 Red Hat, Inc.
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_LINKER_NO_UNDEFINED],[
+AC_MSG_CHECKING([for how to stop undefined symbols at link time])
+
+NO_UNDEFINED_LDFLAGS=
+ld_help=`$LD --help 2>&1`
+case $ld_help in
+*"-z defs"*) NO_UNDEFINED_LDFLAGS="-Wl,-z -Wl,defs" ;;
+esac
+AC_SUBST([NO_UNDEFINED_LDFLAGS])
+
+AC_MSG_RESULT([$NO_UNDEFINED_LDFLAGS])
+])
diff --git a/src/Makefile.am b/src/Makefile.am
index 335b3a0c81..8fddafee51 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -52,6 +52,7 @@ AM_LDFLAGS =  $(DRIVER_MODULES_LDFLAGS) \
$(MINGW_EXTRA_LDFLAGS) \
$(NULL)
 AM_LDFLAGS_MOD = -module -avoid-version $(AM_LDFLAGS)
+AM_LDFLAGS_MOD_NOUNDEF = $(AM_LDFLAGS_MOD) $(NO_UNDEFINED_LDFLAGS)
 
 POD2MAN = pod2man -c "Virtualization Support" -r "$(PACKAGE)-$(VERSION)"
 
@@ -1336,7 +1337,7 @@ libvirt_driver_xen_la_SOURCES =
 libvirt_driver_xen_la_LIBADD = libvirt_driver_xen_impl.la
 mod_LTLIBRARIES += libvirt_driver_xen.la
 libvirt_driver_xen_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_xen_la_LDFLAGS = $(AM_LDFLAGS_MOD)
+libvirt_driver_xen_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF)
 
 libvirt_driver_xen_impl_la_CFLAGS = \
$(XEN_CFLAGS) \
@@ -1382,7 +1383,7 @@ libvirt_driver_vbox_la_LIBADD = 
libvirt_driver_vbox_impl.la
 mod_LTLIBRARIES += \
libvirt_driver_vbox.la
 libvirt_driver_vbox_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
-libvirt_driver_vbox_la_LDFLAGS = $(AM_LDFLAGS_MOD)
+libvirt_driver_vbox_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF)
 
 libvirt_driver_vbox_impl_la_CFLAGS = \
-I$(srcdir)/conf \
@@ -1411,7 +1412,7 @@ libvirt_driver_libxl_la_SOURCES =
 libvirt_driver_libxl_la_LIBADD = libvirt_driver_libxl_impl.la
 mod_LTLIBRARIES += 

[libvirt] [PATCH 11/14] build: explicitly link all modules with libvirt.so

[Daniel P . Berrangé]

The dlopened modules we currently build all use various symbols from
libvirt.so, but don't actually link to it. They rely on the libvirtd
daemon re-exporting the libvirt.so symbols. This means that at the
time the modules are linked, they contain a huge number of undefined
symbols. It also means that these undefined symbols are not versioned,
so despite us providing a LIBVIRT_PRIVATE_ version that
intentionally changes on every release, the loadable modules could
actually be loaded into any libvirtd regardless of version.

This change explicitly links all modules against libvirt.so so
that they don't rely on the re-export behave and can be fully resolved
at build time. This will give us a stronger guarantee modules will
actually be loadable at runtime and that we're using modules from the
matched build.

Signed-off-by: Daniel P. Berrange 
---
 src/Makefile.am | 49 -
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index ec2b4f631c..492bbac22e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1334,7 +1334,7 @@ noinst_LTLIBRARIES += libvirt_driver_xen_impl.la
 libvirt_driver_xen_la_SOURCES =
 libvirt_driver_xen_la_LIBADD = libvirt_driver_xen_impl.la
 mod_LTLIBRARIES += libvirt_driver_xen.la
-libvirt_driver_xen_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_xen_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_xen_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 
 libvirt_driver_xen_impl_la_CFLAGS = \
@@ -1380,7 +1380,7 @@ libvirt_driver_vbox_la_SOURCES =
 libvirt_driver_vbox_la_LIBADD = libvirt_driver_vbox_impl.la
 mod_LTLIBRARIES += \
libvirt_driver_vbox.la
-libvirt_driver_vbox_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_vbox_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_vbox_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 
 libvirt_driver_vbox_impl_la_CFLAGS = \
@@ -1409,7 +1409,7 @@ noinst_LTLIBRARIES += libvirt_driver_libxl_impl.la
 libvirt_driver_libxl_la_SOURCES =
 libvirt_driver_libxl_la_LIBADD = libvirt_driver_libxl_impl.la
 mod_LTLIBRARIES += libvirt_driver_libxl.la
-libvirt_driver_libxl_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_libxl_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_libxl_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 
 libvirt_driver_libxl_impl_la_CFLAGS = \
@@ -1421,8 +1421,7 @@ libvirt_driver_libxl_impl_la_CFLAGS = \
$(AM_CFLAGS)
 libvirt_driver_libxl_impl_la_LDFLAGS = $(AM_LDFLAGS)
 libvirt_driver_libxl_impl_la_LIBADD = $(LIBXL_LIBS) \
- libvirt_xenconfig_libxl.la \
- libvirt_secret.la
+ libvirt_xenconfig_libxl.la
 libvirt_driver_libxl_impl_la_SOURCES = $(LIBXL_DRIVER_SOURCES)
 
 conf_DATA += libxl/libxl.conf
@@ -1439,7 +1438,7 @@ noinst_LTLIBRARIES += libvirt_driver_qemu_impl.la
 libvirt_driver_qemu_la_SOURCES =
 libvirt_driver_qemu_la_LIBADD = libvirt_driver_qemu_impl.la
 mod_LTLIBRARIES += libvirt_driver_qemu.la
-libvirt_driver_qemu_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_qemu_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_qemu_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 
 libvirt_driver_qemu_impl_la_CFLAGS = \
@@ -1455,7 +1454,6 @@ libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \
 $(GNUTLS_LIBS) \
$(LIBNL_LIBS) \
$(LIBXML_LIBS) \
-   libvirt_secret.la \
$(NULL)
 libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
 
@@ -1475,7 +1473,7 @@ noinst_LTLIBRARIES += libvirt_driver_lxc_impl.la
 libvirt_driver_lxc_la_SOURCES =
 libvirt_driver_lxc_la_LIBADD = libvirt_driver_lxc_impl.la
 mod_LTLIBRARIES += libvirt_driver_lxc.la
-libvirt_driver_lxc_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_lxc_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_lxc_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 
 libvirt_driver_lxc_impl_la_CFLAGS = \
@@ -1512,7 +1510,7 @@ noinst_LTLIBRARIES += libvirt_driver_uml_impl.la
 libvirt_driver_uml_la_SOURCES =
 libvirt_driver_uml_la_LIBADD = libvirt_driver_uml_impl.la
 mod_LTLIBRARIES += libvirt_driver_uml.la
-libvirt_driver_uml_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_uml_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_uml_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 
 libvirt_driver_uml_impl_la_CFLAGS = \
@@ -1584,7 +1582,7 @@ noinst_LTLIBRARIES += libvirt_driver_vz_impl.la
 libvirt_driver_vz_la_SOURCES =
 libvirt_driver_vz_la_LIBADD = libvirt_driver_vz_impl.la
 mod_LTLIBRARIES += libvirt_driver_vz.la
-libvirt_driver_vz_la_LIBADD += ../gnulib/lib/libgnu.la
+libvirt_driver_vz_la_LIBADD += libvirt.la ../gnulib/lib/libgnu.la
 libvirt_driver_vz_la_LDFLAGS = 

[libvirt] [PATCH 10/14] storage: export virStoragePoolLookupByTargetPath as a public API

[Daniel P . Berrangé]

The storagePoolLookupByTargetPath() method in the storage driver is used
by the QEMU driver during block migration. If there's a valid use case
for this in the QEMU driver, then external apps likely have similar
needs. Exposing it in the public API removes the direct dependancy from
the QEMU driver to the storage driver.

Signed-off-by: Daniel P. Berrangé 
---
 include/libvirt/libvirt-storage.h |  2 ++
 src/driver-storage.h  |  5 +
 src/libvirt-storage.c | 40 +++
 src/libvirt_public.syms   |  6 ++
 src/qemu/qemu_migration.c |  2 +-
 src/remote/remote_driver.c|  1 +
 src/remote/remote_protocol.x  | 17 -
 src/remote_protocol-structs   |  7 +++
 src/storage/storage_driver.c  |  5 +
 9 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/include/libvirt/libvirt-storage.h 
b/include/libvirt/libvirt-storage.h
index 736e2e3b80..413d9f6c4c 100644
--- a/include/libvirt/libvirt-storage.h
+++ b/include/libvirt/libvirt-storage.h
@@ -264,6 +264,8 @@ virStoragePoolPtr   virStoragePoolLookupByUUID  
(virConnectPtr conn,
 virStoragePoolPtr   virStoragePoolLookupByUUIDString(virConnectPtr conn,
  const char *uuid);
 virStoragePoolPtr   virStoragePoolLookupByVolume(virStorageVolPtr vol);
+virStoragePoolPtr   virStoragePoolLookupByTargetPath(virConnectPtr conn,
+ const char *path);
 
 /*
  * Creating/destroying pools
diff --git a/src/driver-storage.h b/src/driver-storage.h
index 48e588a546..146eb88b2c 100644
--- a/src/driver-storage.h
+++ b/src/driver-storage.h
@@ -63,6 +63,10 @@ typedef virStoragePoolPtr
 typedef virStoragePoolPtr
 (*virDrvStoragePoolLookupByVolume)(virStorageVolPtr vol);
 
+typedef virStoragePoolPtr
+(*virDrvStoragePoolLookupByTargetPath)(virConnectPtr conn,
+   const char *path);
+
 typedef virStoragePoolPtr
 (*virDrvStoragePoolCreateXML)(virConnectPtr conn,
   const char *xmlDesc,
@@ -236,6 +240,7 @@ struct _virStorageDriver {
 virDrvStoragePoolLookupByName storagePoolLookupByName;
 virDrvStoragePoolLookupByUUID storagePoolLookupByUUID;
 virDrvStoragePoolLookupByVolume storagePoolLookupByVolume;
+virDrvStoragePoolLookupByTargetPath storagePoolLookupByTargetPath;
 virDrvStoragePoolCreateXML storagePoolCreateXML;
 virDrvStoragePoolDefineXML storagePoolDefineXML;
 virDrvStoragePoolBuild storagePoolBuild;
diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
index e4646cb80f..3845a5d55e 100644
--- a/src/libvirt-storage.c
+++ b/src/libvirt-storage.c
@@ -497,6 +497,46 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
 }
 
 
+/**
+ * virStoragePoolLookupByTargetPath:
+ * @conn: pointer to hypervisor connection
+ * @path: path at which the pool is exposed
+ *
+ * Fetch a storage pool which maps to a particular target directory.
+ * If more than one pool maps to the path, it is undefined which
+ * will be returned first.
+ *
+ * virStoragePoolFree should be used to free the resources after the
+ * storage pool object is no longer needed.
+ *
+ * Returns a virStoragePoolPtr object, or NULL if no matching pool is found
+ */
+virStoragePoolPtr
+virStoragePoolLookupByTargetPath(virConnectPtr conn,
+ const char *path)
+{
+VIR_DEBUG("conn=%p, path=%s", conn, NULLSTR(path));
+
+virResetLastError();
+
+virCheckConnectReturn(conn, NULL);
+virCheckNonNullArgGoto(path, error);
+
+if (conn->storageDriver && 
conn->storageDriver->storagePoolLookupByTargetPath) {
+virStoragePoolPtr ret;
+ret = conn->storageDriver->storagePoolLookupByTargetPath(conn, path);
+if (!ret)
+goto error;
+return ret;
+}
+
+virReportUnsupportedError();
+
+ error:
+virDispatchError(conn);
+return NULL;
+}
+
 /**
  * virStoragePoolCreateXML:
  * @conn: pointer to hypervisor connection
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 0efde25a7f..7bb8b3dd16 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -779,4 +779,10 @@ LIBVIRT_3.9.0 {
 global:
 virDomainSetLifecycleAction;
 } LIBVIRT_3.7.0;
+
+LIBVIRT_4.1.0 {
+global:
+   virStoragePoolLookupByTargetPath;
+} LIBVIRT_3.9.0;
+
 #  define new API here using predicted next version number 
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1854900c9a..8301c76a19 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -329,7 +329,7 @@ qemuMigrationPrecreateDisk(virConnectPtr conn,
 *volName = '\0';
 volName++;
 
-if (!(pool = storagePoolLookupByTargetPath(conn, basePath)))
+if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
 goto cleanup;
 

[libvirt] [PATCH 09/14] conf: move virStorageTranslateDiskSourcePool into domain conf

[Daniel P . Berrangé]

The virStorageTranslateDiskSourcePool method modifies a virDomainDiskDef
to resolve any storage pool reference. For some reason this was added
into the storage driver code, despite working entirely in terms of the
public APIs. Move it into the domain conf file and rename it to match the
object it modifies.

Signed-off-by: Daniel P. Berrangé 
---
 src/bhyve/bhyve_command.c|   6 +-
 src/conf/domain_conf.c   | 253 +++
 src/conf/domain_conf.h   |   4 +
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   |   8 +-
 src/qemu/qemu_hotplug.c  |   2 +-
 src/qemu/qemu_process.c  |   4 +-
 src/storage/storage_driver.c | 251 --
 src/storage/storage_driver.h |   3 -
 9 files changed, 268 insertions(+), 264 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 55032ae1df..5853a59372 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -199,7 +199,7 @@ bhyveBuildAHCIControllerArgStr(const virDomainDef *def,
 goto error;
 }
 
-if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
+if (virDomainDiskTranslateSourcePool(conn, disk) < 0)
 goto error;
 
 disk_source = virDomainDiskGetSource(disk);
@@ -295,7 +295,7 @@ bhyveBuildVirtIODiskArgStr(const virDomainDef *def 
ATTRIBUTE_UNUSED,
 {
 const char *disk_source;
 
-if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
+if (virDomainDiskTranslateSourcePool(conn, disk) < 0)
 return -1;
 
 if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
@@ -676,7 +676,7 @@ static bool
 virBhyveUsableDisk(virConnectPtr conn, virDomainDiskDefPtr disk)
 {
 
-if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
+if (virDomainDiskTranslateSourcePool(conn, disk) < 0)
 return false;
 
 if ((disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) &&
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 05892c30d9..3d3104bb86 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28544,3 +28544,256 @@ virDomainNetResolveActualType(virDomainNetDefPtr 
iface)
 
 return netResolveActualType(iface);
 }
+
+
+static int
+virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def,
+virStoragePoolDefPtr pooldef)
+{
+int ret = -1;
+char **tokens = NULL;
+
+/* Only support one host */
+if (pooldef->source.nhost != 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Expected exactly 1 host for the storage pool"));
+goto cleanup;
+}
+
+/* iscsi pool only supports one host */
+def->src->nhosts = 1;
+
+if (VIR_ALLOC_N(def->src->hosts, def->src->nhosts) < 0)
+goto cleanup;
+
+if (VIR_STRDUP(def->src->hosts[0].name, pooldef->source.hosts[0].name) < 0)
+goto cleanup;
+
+def->src->hosts[0].port = pooldef->source.hosts[0].port ?
+pooldef->source.hosts[0].port : 3260;
+
+/* iscsi volume has name like "unit:0:0:1" */
+if (!(tokens = virStringSplit(def->src->srcpool->volume, ":", 0)))
+goto cleanup;
+
+if (virStringListLength((const char * const *)tokens) != 4) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("unexpected iscsi volume name '%s'"),
+   def->src->srcpool->volume);
+goto cleanup;
+}
+
+/* iscsi pool has only one source device path */
+if (virAsprintf(>src->path, "%s/%s",
+pooldef->source.devices[0].path,
+tokens[3]) < 0)
+goto cleanup;
+
+/* Storage pool have not supported these 2 attributes yet,
+ * use the defaults.
+ */
+def->src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+def->src->hosts[0].socket = NULL;
+
+def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
+
+ret = 0;
+
+ cleanup:
+virStringListFree(tokens);
+return ret;
+}
+
+
+static int
+virDomainDiskTranslateSourcePoolAuth(virDomainDiskDefPtr def,
+ virStoragePoolSourcePtr source)
+{
+int ret = -1;
+
+/* Only necessary when authentication set */
+if (!source->auth) {
+ret = 0;
+goto cleanup;
+}
+def->src->auth = virStorageAuthDefCopy(source->auth);
+if (!def->src->auth)
+goto cleanup;
+/* A  doesn't use src->auth->authType = VIR_STORAGE_AUTH_TYPE_NONE;
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
+
+int
+virDomainDiskTranslateSourcePool(virConnectPtr conn,
+ virDomainDiskDefPtr def)
+{
+virStoragePoolDefPtr pooldef = NULL;
+virStoragePoolPtr pool = NULL;
+virStorageVolPtr vol = NULL;
+char *poolxml = NULL;
+virStorageVolInfo info;
+int ret = -1;
+
+if (def->src->type != VIR_STORAGE_TYPE_VOLUME)
+return 0;
+
+if (!def->src->srcpool)
+return 0;
+
+  

[libvirt] [PATCH 06/14] qemu: replace networkGetNetworkAddress with public API calls

[Daniel P . Berrangé]

The QEMU driver calls into the network driver to get the first IP
address of the network. This information is readily available via the
formal public API by fetching the XML doc and then parsing it.

Signed-off-by: Daniel P. Berrangé 
---
 src/network/bridge_driver.c |  98 -
 src/network/bridge_driver.h |   6 ---
 src/qemu/qemu_process.c | 103 +---
 3 files changed, 96 insertions(+), 111 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a0e45d1f65..ee200f1343 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5104,104 +5104,6 @@ networkReleaseActualDevice(virDomainDefPtr dom,
 }
 
 
-/*
- * networkGetNetworkAddress:
- * @netname: the name of a network
- * @netaddr: string representation of IP address for that network.
- *
- * Attempt to return an IP address associated with the named
- * network. If a libvirt virtual network, that will be provided in the
- * configuration. For host bridge and direct (macvtap) networks, we
- * must do an ioctl to learn the address.
- *
- * Note: This function returns the first IP address it finds. It might
- * be useful if it was more flexible, but the current use (getting a
- * listen address for qemu's vnc/spice graphics server) can only use a
- * single address anyway.
- *
- * Returns 0 on success, and puts a string (which must be free'd by
- * the caller) into *netaddr. Returns -1 on failure or -2 if
- * completely unsupported.
- */
-int
-networkGetNetworkAddress(const char *netname,
- char **netaddr)
-{
-virNetworkDriverStatePtr driver = networkGetDriver();
-int ret = -1;
-virNetworkObjPtr obj;
-virNetworkDefPtr netdef;
-virNetworkIPDefPtr ipdef;
-virSocketAddr addr;
-virSocketAddrPtr addrptr = NULL;
-char *dev_name = NULL;
-
-*netaddr = NULL;
-obj = virNetworkObjFindByName(driver->networks, netname);
-if (!obj) {
-virReportError(VIR_ERR_NO_NETWORK,
-   _("no network with matching name '%s'"),
-   netname);
-goto cleanup;
-}
-netdef = virNetworkObjGetDef(obj);
-
-switch (netdef->forward.type) {
-case VIR_NETWORK_FORWARD_NONE:
-case VIR_NETWORK_FORWARD_NAT:
-case VIR_NETWORK_FORWARD_ROUTE:
-case VIR_NETWORK_FORWARD_OPEN:
-ipdef = virNetworkDefGetIPByIndex(netdef, AF_UNSPEC, 0);
-if (!ipdef) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("network '%s' doesn't have an IP address"),
-   netdef->name);
-goto cleanup;
-}
-addrptr = >address;
-break;
-
-case VIR_NETWORK_FORWARD_BRIDGE:
-if ((dev_name = netdef->bridge))
-break;
-/*
- * fall through if netdef->bridge wasn't set, since that is
- * macvtap bridge mode network.
- */
-ATTRIBUTE_FALLTHROUGH;
-
-case VIR_NETWORK_FORWARD_PRIVATE:
-case VIR_NETWORK_FORWARD_VEPA:
-case VIR_NETWORK_FORWARD_PASSTHROUGH:
-if ((netdef->forward.nifs > 0) && netdef->forward.ifs)
-dev_name = netdef->forward.ifs[0].device.dev;
-
-if (!dev_name) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("network '%s' has no associated interface or 
bridge"),
-   netdef->name);
-goto cleanup;
-}
-break;
-}
-
-if (dev_name) {
-if (virNetDevIPAddrGet(dev_name, ) < 0)
-goto cleanup;
-addrptr = 
-}
-
-if (!(addrptr &&
-  (*netaddr = virSocketAddrFormat(addrptr {
-goto cleanup;
-}
-
-ret = 0;
- cleanup:
-virNetworkObjEndAPI();
-return ret;
-}
-
 
 /* networkGetActualType:
  * @dom: domain definition that @iface belongs to
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 098f9e5c5b..6fa6432d13 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -36,11 +36,6 @@ networkRegister(void);
 
 # if WITH_NETWORK
 
-int
-networkGetNetworkAddress(const char *netname,
- char **netaddr)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-
 int
 networkGetActualType(virDomainNetDefPtr iface)
 ATTRIBUTE_NONNULL(1);
@@ -55,7 +50,6 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
 # else
 /* Define no-op replacements that don't drag in any link dependencies.  */
 #  define networkGetActualType(iface) (iface->type)
-#  define networkGetNetworkAddress(netname, netaddr) (-2)
 #  define networkDnsmasqConfContents(network, pidfile, configstr, \
 dctx, caps) 0
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 29af643160..2c352523fe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4259,9 +4259,95 @@ 

[libvirt] [PATCH 07/14] conf: expand network device callbacks to cover resolving NIC type

[Daniel P . Berrangé]

Currently the QEMU driver will call directly into the network driver
impl to modify resolve the atual type of NICs with type=network. It
has todo this before it has allocated the actual NIC. This introduces
a callback system to allow us to decouple the QEMU driver from the
network driver.

This is a short term step, as it ought to be possible to achieve the
same end goal by simply querying XML via the public network API. The
QEMU code in question though, has no virConnectPtr conveniently
available at this time.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c | 17 -
 src/conf/domain_conf.h | 15 ++-
 src/libvirt_private.syms   |  1 +
 src/network/bridge_driver.c| 10 +-
 src/network/bridge_driver.h|  5 -
 src/qemu/qemu_alias.c  |  2 +-
 src/qemu/qemu_domain_address.c |  2 +-
 7 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5cae6f51cf..05892c30d9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28448,6 +28448,7 @@ static virDomainNetNotifyActualDeviceImpl netNotify;
 static virDomainNetReleaseActualDeviceImpl netRelease;
 static virDomainNetBandwidthChangeAllowedImpl netBandwidthChangeAllowed;
 static virDomainNetBandwidthUpdateImpl netBandwidthUpdate;
+static virDomainNetResolveActualTypeImpl netResolveActualType;
 
 
 void
@@ -28455,13 +28456,15 @@ 
virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
   virDomainNetNotifyActualDeviceImpl notify,
   virDomainNetReleaseActualDeviceImpl release,
   virDomainNetBandwidthChangeAllowedImpl 
bandwidthChangeAllowed,
-  virDomainNetBandwidthUpdateImpl bandwidthUpdate)
+  virDomainNetBandwidthUpdateImpl bandwidthUpdate,
+  virDomainNetResolveActualTypeImpl resolveActualType)
 {
 netAllocate = allocate;
 netNotify = notify;
 netRelease = release;
 netBandwidthChangeAllowed = bandwidthChangeAllowed;
 netBandwidthUpdate = bandwidthUpdate;
+netResolveActualType = resolveActualType;
 }
 
 int
@@ -28529,3 +28532,15 @@ virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
 
 return netBandwidthUpdate(iface, newBandwidth);
 }
+
+int
+virDomainNetResolveActualType(virDomainNetDefPtr iface)
+{
+if (!netResolveActualType) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Network device resolve type not available"));
+return -1;
+}
+
+return netResolveActualType(iface);
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e876fe953d..d1f4038295 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3461,13 +3461,17 @@ typedef int
 (*virDomainNetBandwidthUpdateImpl)(virDomainNetDefPtr iface,
virNetDevBandwidthPtr newBandwidth);
 
+typedef int
+(*virDomainNetResolveActualTypeImpl)(virDomainNetDefPtr iface);
+
 
 void
 virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
   virDomainNetNotifyActualDeviceImpl notify,
   virDomainNetReleaseActualDeviceImpl release,
   virDomainNetBandwidthChangeAllowedImpl 
bandwidthChangeAllowed,
-  virDomainNetBandwidthUpdateImpl bandwidthUpdate);
+  virDomainNetBandwidthUpdateImpl bandwidthUpdate,
+  virDomainNetResolveActualTypeImpl resolveActualType);
 
 int
 virDomainNetAllocateActualDevice(virDomainDefPtr dom,
@@ -3494,5 +3498,14 @@ virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
 virNetDevBandwidthPtr newBandwidth)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
+/* XXX this is a nasty hack and should be removed. It should
+ * be by via public API by fetching XML and parsing it. Not
+ * easy right now as code paths in QEMU reying on this don't
+ * have a virConnectPtr handy.
+ */
+int
+virDomainNetResolveActualType(virDomainNetDefPtr iface)
+ATTRIBUTE_NONNULL(1);
+
 
 #endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 10d8e65b62..fc68c9d33e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -456,6 +456,7 @@ virDomainNetNotifyActualDevice;
 virDomainNetReleaseActualDevice;
 virDomainNetRemove;
 virDomainNetRemoveHostdev;
+virDomainNetResolveActualType;
 virDomainNetSetDeviceImpl;
 virDomainNetTypeFromString;
 virDomainNetTypeSharesHostView;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ee200f1343..76747670e9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5105,8 +5105,7 @@ networkReleaseActualDevice(virDomainDefPtr dom,
 
 
 
-/* networkGetActualType:
- * @dom: domain definition that @iface belongs to
+/* 

[libvirt] [PATCH 08/14] network: remove conditional declarations

[Daniel P . Berrangé]

The networkDnsmasqConfContents() method is only used by the test suite
and that's only built with WITH_NETWORK is set. So there is no longer
any reason to conditionalize the declaration of this method.

Signed-off-by: Daniel P. Berrangé 
---
 src/network/bridge_driver.h | 9 -
 1 file changed, 9 deletions(-)

diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 2cf886a7e6..b70881a690 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -34,8 +34,6 @@
 int
 networkRegister(void);
 
-# if WITH_NETWORK
-
 int
 networkDnsmasqConfContents(virNetworkObjPtr obj,
const char *pidfile,
@@ -43,11 +41,4 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
dnsmasqContext *dctx,
dnsmasqCapsPtr caps);
 
-# else
-/* Define no-op replacements that don't drag in any link dependencies.  */
-#  define networkDnsmasqConfContents(network, pidfile, configstr, \
-dctx, caps) 0
-
-# endif
-
 #endif /* __VIR_NETWORK__DRIVER_H */
-- 
2.14.3

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

[libvirt] [PATCH 04/14] conf: introduce callback registration for domain net device allocation

[Daniel P . Berrangé]

Currently virt drivers will call directly into the network driver impl
to allocate domain interface devices where type=network. This introduces
a callback system to allow us to decouple the virt drivers from the
network driver.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c  | 54 +
 src/conf/domain_conf.h  | 34 
 src/libvirt_private.syms|  4 
 src/libxl/libxl_domain.c|  4 ++--
 src/libxl/libxl_driver.c|  6 ++---
 src/lxc/lxc_driver.c|  4 ++--
 src/lxc/lxc_process.c   |  6 ++---
 src/network/bridge_driver.c | 32 ---
 src/network/bridge_driver.h | 28 ---
 src/qemu/qemu_hotplug.c | 14 ++--
 src/qemu/qemu_process.c |  6 ++---
 11 files changed, 131 insertions(+), 61 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a1c25060f9..a3719164ae 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28442,3 +28442,57 @@ virDomainNetTypeSharesHostView(const virDomainNetDef 
*net)
 }
 return false;
 }
+
+static virDomainNetAllocateActualDeviceImpl netAllocate;
+static virDomainNetNotifyActualDeviceImpl netNotify;
+static virDomainNetReleaseActualDeviceImpl netRelease;
+
+void
+virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
+  virDomainNetNotifyActualDeviceImpl notify,
+  virDomainNetReleaseActualDeviceImpl release)
+{
+netAllocate = allocate;
+netNotify = notify;
+netRelease = release;
+}
+
+int
+virDomainNetAllocateActualDevice(virDomainDefPtr dom,
+ virDomainNetDefPtr iface)
+{
+if (!netAllocate) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Network device allocation not available"));
+return -1;
+}
+
+return netAllocate(dom, iface);
+}
+
+void
+virDomainNetNotifyActualDevice(virDomainDefPtr dom,
+   virDomainNetDefPtr iface)
+{
+if (!netNotify) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Network device notification not available"));
+return;
+}
+
+netNotify(dom, iface);
+}
+
+
+int
+virDomainNetReleaseActualDevice(virDomainDefPtr dom,
+virDomainNetDefPtr iface)
+{
+if (!netRelease) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Network device release not available"));
+return -1;
+}
+
+return netRelease(dom, iface);
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6f7f96b3dd..1d1e3ac1a4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3441,4 +3441,38 @@ bool
 virDomainDefLifecycleActionAllowed(virDomainLifecycle type,
virDomainLifecycleAction action);
 
+typedef int
+(*virDomainNetAllocateActualDeviceImpl)(virDomainDefPtr dom,
+virDomainNetDefPtr iface);
+
+typedef void
+(*virDomainNetNotifyActualDeviceImpl)(virDomainDefPtr dom,
+  virDomainNetDefPtr iface);
+
+typedef int
+(*virDomainNetReleaseActualDeviceImpl)(virDomainDefPtr dom,
+   virDomainNetDefPtr iface);
+
+void
+virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
+  virDomainNetNotifyActualDeviceImpl notify,
+  virDomainNetReleaseActualDeviceImpl release);
+
+int
+virDomainNetAllocateActualDevice(virDomainDefPtr dom,
+ virDomainNetDefPtr iface)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+void
+virDomainNetNotifyActualDevice(virDomainDefPtr dom,
+   virDomainNetDefPtr iface)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+int
+virDomainNetReleaseActualDevice(virDomainDefPtr dom,
+virDomainNetDefPtr iface)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+
+
 #endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3d8ed0b9de..831b030b8c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -430,6 +430,7 @@ virDomainMemoryInsert;
 virDomainMemoryRemove;
 virDomainMemorySourceTypeFromString;
 virDomainMemorySourceTypeToString;
+virDomainNetAllocateActualDevice;
 virDomainNetAppendIPAddress;
 virDomainNetDefClear;
 virDomainNetDefFormat;
@@ -449,8 +450,11 @@ virDomainNetGetActualType;
 virDomainNetGetActualVirtPortProfile;
 virDomainNetGetActualVlan;
 virDomainNetInsert;
+virDomainNetNotifyActualDevice;
+virDomainNetReleaseActualDevice;
 virDomainNetRemove;
 virDomainNetRemoveHostdev;
+virDomainNetSetDeviceImpl;
 virDomainNetTypeFromString;
 virDomainNetTypeSharesHostView;
 virDomainNetTypeToString;
diff --git a/src/libxl/libxl_domain.c 

[libvirt] [PATCH 05/14] conf: expand network device callbacks to cover bandwidth updates

[Daniel P . Berrangé]

Currently the QEMU driver will call directly into the network driver
impl to modify network device bandwidth for interfaces with
type=network. This introduces a callback system to allow us to decouple
the QEMU driver from the network driver.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c  | 35 ++-
 src/conf/domain_conf.h  | 22 +-
 src/libvirt_private.syms|  2 ++
 src/network/bridge_driver.c |  8 +---
 src/network/bridge_driver.h | 24 
 src/qemu/qemu_driver.c  |  4 ++--
 6 files changed, 64 insertions(+), 31 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3719164ae..5cae6f51cf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28446,15 +28446,22 @@ virDomainNetTypeSharesHostView(const virDomainNetDef 
*net)
 static virDomainNetAllocateActualDeviceImpl netAllocate;
 static virDomainNetNotifyActualDeviceImpl netNotify;
 static virDomainNetReleaseActualDeviceImpl netRelease;
+static virDomainNetBandwidthChangeAllowedImpl netBandwidthChangeAllowed;
+static virDomainNetBandwidthUpdateImpl netBandwidthUpdate;
+
 
 void
 virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
   virDomainNetNotifyActualDeviceImpl notify,
-  virDomainNetReleaseActualDeviceImpl release)
+  virDomainNetReleaseActualDeviceImpl release,
+  virDomainNetBandwidthChangeAllowedImpl 
bandwidthChangeAllowed,
+  virDomainNetBandwidthUpdateImpl bandwidthUpdate)
 {
 netAllocate = allocate;
 netNotify = notify;
 netRelease = release;
+netBandwidthChangeAllowed = bandwidthChangeAllowed;
+netBandwidthUpdate = bandwidthUpdate;
 }
 
 int
@@ -28496,3 +28503,29 @@ virDomainNetReleaseActualDevice(virDomainDefPtr dom,
 
 return netRelease(dom, iface);
 }
+
+bool
+virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
+  virNetDevBandwidthPtr newBandwidth)
+{
+if (!netBandwidthChangeAllowed) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Network device release not available"));
+return -1;
+}
+
+return netBandwidthChangeAllowed(iface, newBandwidth);
+}
+
+int
+virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
+virNetDevBandwidthPtr newBandwidth)
+{
+if (!netBandwidthUpdate) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Network device release not available"));
+return -1;
+}
+
+return netBandwidthUpdate(iface, newBandwidth);
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1d1e3ac1a4..e876fe953d 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3453,10 +3453,21 @@ typedef int
 (*virDomainNetReleaseActualDeviceImpl)(virDomainDefPtr dom,
virDomainNetDefPtr iface);
 
+typedef bool
+(*virDomainNetBandwidthChangeAllowedImpl)(virDomainNetDefPtr iface,
+  virNetDevBandwidthPtr newBandwidth);
+
+typedef int
+(*virDomainNetBandwidthUpdateImpl)(virDomainNetDefPtr iface,
+   virNetDevBandwidthPtr newBandwidth);
+
+
 void
 virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
   virDomainNetNotifyActualDeviceImpl notify,
-  virDomainNetReleaseActualDeviceImpl release);
+  virDomainNetReleaseActualDeviceImpl release,
+  virDomainNetBandwidthChangeAllowedImpl 
bandwidthChangeAllowed,
+  virDomainNetBandwidthUpdateImpl bandwidthUpdate);
 
 int
 virDomainNetAllocateActualDevice(virDomainDefPtr dom,
@@ -3473,6 +3484,15 @@ virDomainNetReleaseActualDevice(virDomainDefPtr dom,
 virDomainNetDefPtr iface)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
+bool
+virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
+  virNetDevBandwidthPtr newBandwidth)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
+int
+virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
+virNetDevBandwidthPtr newBandwidth)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 
 #endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 831b030b8c..10d8e65b62 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -432,6 +432,8 @@ virDomainMemorySourceTypeFromString;
 virDomainMemorySourceTypeToString;
 virDomainNetAllocateActualDevice;
 virDomainNetAppendIPAddress;
+virDomainNetBandwidthChangeAllowed;
+virDomainNetBandwidthUpdate;
 virDomainNetDefClear;
 virDomainNetDefFormat;
 virDomainNetDefFree;
diff --git a/src/network/bridge_driver.c 

[libvirt] [PATCH 03/14] rpc: don't link in second copy of RPC code to libvirtd & lockd plugin

[Daniel P . Berrangé]

The libvirt_driver_remote.la static library is linked into the
libvirt.so dynamic library, providing both the generic RPC layer code
and the remote protocol client driver. The libvirtd daemon the itself
links to libvirt_driver_remote.la, in order to get access to the generic
RPC layer code and the XDR functions for the remote driver. This means
we get multiple copies of the same code in libvirtd, one direct and one
indirect via libvirt.so. The same mistake affects the lockd plugin.

The libvirtd daemon should instead just link aganist the generic RPC
layer code that's in libvirt.so. This is easily doable if we add exports
for the few symbols we've previously missed, and wildcard export xdr_*
to expose the auto-generated XDR marshallers.

Signed-off-by: Daniel P. Berrangé 
---
 daemon/Makefile.am  |  1 -
 src/Makefile.am | 12 +---
 src/libvirt_remote.syms | 11 ++-
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index b0c28d2313..f7519efe2f 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -205,7 +205,6 @@ libvirtd_LDADD += \
libvirtd_admin.la \
../src/libvirt-lxc.la \
../src/libvirt-qemu.la \
-   ../src/libvirt_driver_remote.la \
$(NULL)
 
 libvirtd_LDADD += ../src/libvirt.la
diff --git a/src/Makefile.am b/src/Makefile.am
index 6b4db22937..ec2b4f631c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1310,16 +1310,11 @@ if WITH_REMOTE
 noinst_LTLIBRARIES += libvirt_driver_remote.la
 libvirt_la_BUILT_LIBADD += libvirt_driver_remote.la
 libvirt_driver_remote_la_CFLAGS = \
-   $(GNUTLS_CFLAGS) \
$(XDR_CFLAGS) \
-I$(srcdir)/conf \
-I$(srcdir)/rpc \
$(AM_CFLAGS)
 libvirt_driver_remote_la_LDFLAGS = $(AM_LDFLAGS)
-libvirt_driver_remote_la_LIBADD = $(GNUTLS_LIBS) \
-libvirt-net-rpc-client.la \
-libvirt-net-rpc-server.la \
-libvirt-net-rpc.la
 libvirt_driver_remote_la_SOURCES = $(REMOTE_DRIVER_SOURCES)
 
 BUILT_SOURCES += $(REMOTE_DRIVER_GENERATED)
@@ -2574,8 +2569,6 @@ lockd_la_CFLAGS = -I$(srcdir)/conf \
$(AM_CFLAGS)
 lockd_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS)
 lockd_la_LIBADD = ../gnulib/lib/libgnu.la \
-   libvirt-net-rpc.la \
-   libvirt-net-rpc-client.la \
$(NULL)
 augeas_DATA += locking/libvirt_lockd.aug
 if WITH_DTRACE_PROBES
@@ -2889,6 +2882,11 @@ noinst_LTLIBRARIES += \
libvirt-net-rpc-server.la \
libvirt-net-rpc-client.la
 
+libvirt_la_BUILT_LIBADD += \
+   libvirt-net-rpc.la \
+   libvirt-net-rpc-server.la \
+   libvirt-net-rpc-client.la
+
 EXTRA_DIST += \
dtrace2systemtap.pl \
rpc/gendispatch.pl \
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index a181c4cf7f..da14a2449c 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -5,6 +5,9 @@
 # Keep this file sorted by header name, then by symbols with each header.
 #
 
+# Generated files
+xdr_*;
+
 # rpc/virnetclient.h
 virNetClientAddProgram;
 virNetClientAddStream;
@@ -80,6 +83,7 @@ virNetDaemonUpdateServices;
 
 
 # rpc/virnetmessage.h
+virNetMessageAddFD;
 virNetMessageClear;
 virNetMessageClearPayload;
 virNetMessageDecodeHeader;
@@ -96,7 +100,6 @@ virNetMessageNew;
 virNetMessageQueuePush;
 virNetMessageQueueServe;
 virNetMessageSaveError;
-xdr_virNetMessageError;
 
 
 # rpc/virnetserver.h
@@ -104,12 +107,14 @@ virNetServerAddClient;
 virNetServerAddProgram;
 virNetServerAddService;
 virNetServerClose;
+virNetServerGetClient;
 virNetServerGetClients;
 virNetServerGetCurrentClients;
 virNetServerGetCurrentUnauthClients;
 virNetServerGetMaxClients;
 virNetServerGetMaxUnauthClients;
 virNetServerGetName;
+virNetServerGetThreadPoolParameters;
 virNetServerHasClients;
 virNetServerNew;
 virNetServerNewPostExecRestart;
@@ -117,6 +122,8 @@ virNetServerNextClientID;
 virNetServerPreExecRestart;
 virNetServerProcessClients;
 virNetServerSetClientAuthenticated;
+virNetServerSetClientLimits;
+virNetServerSetThreadPoolParameters;
 virNetServerStart;
 virNetServerUpdateServices;
 
@@ -128,11 +135,13 @@ virNetServerClientCloseLocked;
 virNetServerClientDelayedClose;
 virNetServerClientGetAuth;
 virNetServerClientGetFD;
+virNetServerClientGetID;
 virNetServerClientGetIdentity;
 virNetServerClientGetInfo;
 virNetServerClientGetPrivateData;
 virNetServerClientGetReadonly;
 virNetServerClientGetSELinuxContext;
+virNetServerClientGetTimestamp;
 virNetServerClientGetTransport;
 virNetServerClientGetUNIXIdentity;
 virNetServerClientImmediateClose;
-- 
2.14.3

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

[libvirt] [PATCH 01/14] storage: extract storage file backend from main storage driver backend

[Daniel P . Berrangé]

The storage driver backends are serving the public storage pools API,
while the storage file backends are serving the internal QEMU driver and
/ or libvirt utility code.

To prep for moving this storage file backend framework into the utility
code, split out the backend definitions.

Signed-off-by: Daniel P. Berrange 
---
 po/POTFILES.in|   1 +
 src/Makefile.am   |   1 +
 src/storage/storage_backend.c |  66 -
 src/storage/storage_backend.h |  75 ---
 src/storage/storage_backend_fs.c  |   7 ++-
 src/storage/storage_backend_gluster.c |   3 +-
 src/storage/storage_source.c  |   2 +-
 src/storage/storage_source_backend.c  | 108 ++
 src/storage/storage_source_backend.h  | 104 
 9 files changed, 221 insertions(+), 146 deletions(-)
 create mode 100644 src/storage/storage_source_backend.c
 create mode 100644 src/storage/storage_source_backend.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index c1fa23427e..7145af7176 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -187,6 +187,7 @@ src/storage/storage_backend_vstorage.c
 src/storage/storage_backend_zfs.c
 src/storage/storage_driver.c
 src/storage/storage_source.c
+src/storage/storage_source_backend.c
 src/storage/storage_util.c
 src/test/test_driver.c
 src/uml/uml_conf.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 166c9a8e91..412eb10a4a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1057,6 +1057,7 @@ STORAGE_DRIVER_BACKEND_SOURCES = \
 STORAGE_DRIVER_SOURCES = \
storage/storage_driver.h storage/storage_driver.c \
storage/storage_source.h storage/storage_source.c \
+   storage/storage_source_backend.h 
storage/storage_source_backend.c \
$(STORAGE_DRIVER_BACKEND_SOURCES) \
storage/storage_util.h storage/storage_util.c
 
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 5a8c4f7f6a..053f4ecf26 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -78,8 +78,6 @@ VIR_LOG_INIT("storage.storage_backend");
 
 static virStorageBackendPtr virStorageBackends[VIR_STORAGE_BACKENDS_MAX];
 static size_t virStorageBackendsCount;
-static virStorageFileBackendPtr 
virStorageFileBackends[VIR_STORAGE_BACKENDS_MAX];
-static size_t virStorageFileBackendsCount;
 
 #define STORAGE_BACKEND_MODULE_DIR LIBDIR "/libvirt/storage-backend"
 
@@ -179,27 +177,6 @@ virStorageBackendRegister(virStorageBackendPtr backend)
 }
 
 
-int
-virStorageBackendFileRegister(virStorageFileBackendPtr backend)
-{
-VIR_DEBUG("Registering storage file backend '%s' protocol '%s'",
-  virStorageTypeToString(backend->type),
-  virStorageNetProtocolTypeToString(backend->protocol));
-
-if (virStorageFileBackendsCount >= VIR_STORAGE_BACKENDS_MAX) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Too many drivers, cannot register storage file "
- "backend '%s'"),
-   virStorageTypeToString(backend->type));
-return -1;
-}
-
-virStorageFileBackends[virStorageFileBackendsCount] = backend;
-virStorageFileBackendsCount++;
-return 0;
-}
-
-
 virStorageBackendPtr
 virStorageBackendForType(int type)
 {
@@ -213,46 +190,3 @@ virStorageBackendForType(int type)
type, NULLSTR(virStoragePoolTypeToString(type)));
 return NULL;
 }
-
-
-virStorageFileBackendPtr
-virStorageFileBackendForTypeInternal(int type,
- int protocol,
- bool report)
-{
-size_t i;
-
-for (i = 0; i < virStorageFileBackendsCount; i++) {
-if (virStorageFileBackends[i]->type == type) {
-if (type == VIR_STORAGE_TYPE_NETWORK &&
-virStorageFileBackends[i]->protocol != protocol)
-continue;
-
-return virStorageFileBackends[i];
-}
-}
-
-if (!report)
-return NULL;
-
-if (type == VIR_STORAGE_TYPE_NETWORK) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("missing storage backend for network files "
- "using %s protocol"),
-   virStorageNetProtocolTypeToString(protocol));
-} else {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("missing storage backend for '%s' storage"),
-   virStorageTypeToString(type));
-}
-
-return NULL;
-}
-
-
-virStorageFileBackendPtr
-virStorageFileBackendForType(int type,
- int protocol)
-{
-return virStorageFileBackendForTypeInternal(type, protocol, true);
-}
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 193cf134d6..f9aa4cd26b 100644
--- a/src/storage/storage_backend.h
+++ 

[libvirt] [PATCH 02/14] storage: move storage file backend framework into util directory

[Daniel P . Berrangé]

The QEMU driver loadable module needs to be able to resolve all ELF
symbols it references against libvirt.so. Some of its symbols can only
be resolved against the storage_driver.so loadable module which creates
a hard dependancy between them. By moving the storage file backend
framework into the util directory, this gets included directly in the
libvirt.so library. The actual backend implementations are still done as
loadable modules, so this doesn't re-add deps on gluster libraries.

Signed-off-by: Daniel P. Berrange 
---
 po/POTFILES.in |   3 +-
 src/Makefile.am|   3 +-
 src/libvirt_private.syms   |  19 +
 src/qemu/qemu_domain.c |   1 -
 src/qemu/qemu_driver.c |   1 -
 src/security/virt-aa-helper.c  |   2 -
 src/storage/storage_backend_fs.c   |   3 +-
 src/storage/storage_backend_gluster.c  |   3 +-
 src/storage/storage_source.c   | 645 -
 src/storage/storage_source.h   |  59 --
 src/util/virstoragefile.c  | 609 ++-
 src/util/virstoragefile.h  |  32 +
 .../virstoragefilebackend.c}   |   4 +-
 .../virstoragefilebackend.h}   |   8 +-
 tests/virstoragetest.c |   1 -
 15 files changed, 669 insertions(+), 724 deletions(-)
 delete mode 100644 src/storage/storage_source.c
 delete mode 100644 src/storage/storage_source.h
 rename src/{storage/storage_source_backend.c => util/virstoragefilebackend.c} 
(96%)
 rename src/{storage/storage_source_backend.h => util/virstoragefilebackend.h} 
(94%)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 7145af7176..5d88c04de2 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -186,8 +186,6 @@ src/storage/storage_backend_sheepdog.c
 src/storage/storage_backend_vstorage.c
 src/storage/storage_backend_zfs.c
 src/storage/storage_driver.c
-src/storage/storage_source.c
-src/storage/storage_source_backend.c
 src/storage/storage_util.c
 src/test/test_driver.c
 src/uml/uml_conf.c
@@ -262,6 +260,7 @@ src/util/virsexpr.c
 src/util/virsocketaddr.c
 src/util/virstorageencryption.c
 src/util/virstoragefile.c
+src/util/virstoragefilebackend.c
 src/util/virstring.c
 src/util/virsysinfo.c
 src/util/virthreadjob.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 412eb10a4a..6b4db22937 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -178,6 +178,7 @@ UTIL_SOURCES = \
util/virsocketaddr.h util/virsocketaddr.c \
util/virstorageencryption.c util/virstorageencryption.h \
util/virstoragefile.c util/virstoragefile.h \
+   util/virstoragefilebackend.c util/virstoragefilebackend.h \
util/virstring.h util/virstring.c \
util/virsysinfo.c util/virsysinfo.h util/virsysinfopriv.h \
util/virsystemd.c util/virsystemd.h util/virsystemdpriv.h \
@@ -1056,8 +1057,6 @@ STORAGE_DRIVER_BACKEND_SOURCES = \
 
 STORAGE_DRIVER_SOURCES = \
storage/storage_driver.h storage/storage_driver.c \
-   storage/storage_source.h storage/storage_source.c \
-   storage/storage_source_backend.h 
storage/storage_source_backend.c \
$(STORAGE_DRIVER_BACKEND_SOURCES) \
storage/storage_util.h storage/storage_util.c
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bc8cc1fba9..3d8ed0b9de 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2691,24 +2691,39 @@ virStorageAuthDefCopy;
 virStorageAuthDefFormat;
 virStorageAuthDefFree;
 virStorageAuthDefParse;
+virStorageFileAccess;
 virStorageFileCanonicalizePath;
 virStorageFileChainGetBroken;
 virStorageFileChainLookup;
+virStorageFileChown;
+virStorageFileCreate;
+virStorageFileDeinit;
 virStorageFileFeatureTypeFromString;
 virStorageFileFeatureTypeToString;
 virStorageFileFormatTypeFromString;
 virStorageFileFormatTypeToString;
+virStorageFileGetBackingStoreStr;
 virStorageFileGetLVMKey;
+virStorageFileGetMetadata;
 virStorageFileGetMetadataFromBuf;
 virStorageFileGetMetadataFromFD;
 virStorageFileGetMetadataInternal;
 virStorageFileGetRelativeBackingPath;
 virStorageFileGetSCSIKey;
+virStorageFileGetUniqueIdentifier;
+virStorageFileInit;
+virStorageFileInitAs;
 virStorageFileIsClusterFS;
 virStorageFileParseBackingStoreStr;
 virStorageFileParseChainIndex;
 virStorageFileProbeFormat;
+virStorageFileRead;
+virStorageFileReportBrokenChain;
 virStorageFileResize;
+virStorageFileStat;
+virStorageFileSupportsAccess;
+virStorageFileSupportsSecurityDriver;
+virStorageFileUnlink;
 virStorageIsFile;
 virStorageIsRelative;
 virStorageNetHostDefClear;
@@ -2747,6 +2762,10 @@ virStorageTypeFromString;
 virStorageTypeToString;
 
 
+# util/virstoragefilebackend.h

[libvirt] [PATCH 00/14] Misc build refactoring / isolation work

[Daniel P . Berrangé]

This was triggered by the recent Fedora change to add '-z defs' to RPM
builds by default which breaks libvirt. Various make rule changes can
fix much of the problem, but it also requires source refactoring to get
rid of places where virt drivers directly call into the storage/network
drivers. Co-incidentally this work will also be useful in allowing us to
separate out drivers to distinct daemons.

Daniel P. Berrangé (14):
  storage: extract storage file backend from main storage driver backend
  storage: move storage file backend framework into util directory
  rpc: don't link in second copy of RPC code to libvirtd & lockd plugin
  conf: introduce callback registration for domain net device allocation
  conf: expand network device callbacks to cover bandwidth updates
  qemu: replace networkGetNetworkAddress with public API calls
  conf: expand network device callbacks to cover resolving NIC type
  network: remove conditional declarations
  conf: move virStorageTranslateDiskSourcePool into domain conf
  storage: export virStoragePoolLookupByTargetPath as a public API
  build: explicitly link all modules with libvirt.so
  build: provide a AM_FLAGS_MOD for loadable modules
  build: passing the "-z defs" linker flag to prevent undefined symbols
  build: link libvirt_lxc against libvirt.so

 configure.ac  |   1 +
 daemon/Makefile.am|   4 +-
 include/libvirt/libvirt-storage.h |   2 +
 m4/virt-linker-no-undefined.m4|  32 ++
 po/POTFILES.in|   2 +-
 src/Makefile.am   | 142 
 src/bhyve/bhyve_command.c |   6 +-
 src/conf/domain_conf.c| 355 +++
 src/conf/domain_conf.h|  71 
 src/driver-storage.h  |   5 +
 src/libvirt-storage.c |  40 +++
 src/libvirt_private.syms  |  29 ++
 src/libvirt_public.syms   |   6 +
 src/libvirt_remote.syms   |  11 +-
 src/libxl/libxl_domain.c  |   4 +-
 src/libxl/libxl_driver.c  |   6 +-
 src/lxc/lxc_driver.c  |   4 +-
 src/lxc/lxc_process.c |   6 +-
 src/network/bridge_driver.c   | 144 ++--
 src/network/bridge_driver.h   |  72 
 src/qemu/qemu_alias.c |   2 +-
 src/qemu/qemu_domain.c|   1 -
 src/qemu/qemu_domain_address.c|   2 +-
 src/qemu/qemu_driver.c|  13 +-
 src/qemu/qemu_hotplug.c   |  16 +-
 src/qemu/qemu_migration.c |   2 +-
 src/qemu/qemu_process.c   | 113 +-
 src/remote/remote_driver.c|   1 +
 src/remote/remote_protocol.x  |  17 +-
 src/remote_protocol-structs   |   7 +
 src/security/virt-aa-helper.c |   2 -
 src/storage/storage_backend.c |  66 
 src/storage/storage_backend.h |  75 
 src/storage/storage_backend_fs.c  |   8 +-
 src/storage/storage_backend_gluster.c |   4 +-
 src/storage/storage_driver.c  | 256 +-
 src/storage/storage_driver.h  |   3 -
 src/storage/storage_source.c  | 645 --
 src/storage/storage_source.h  |  59 
 src/util/virstoragefile.c | 609 +++-
 src/util/virstoragefile.h |  32 ++
 src/util/virstoragefilebackend.c  | 108 ++
 src/util/virstoragefilebackend.h  | 104 ++
 tests/virstoragetest.c|   1 -
 tools/Makefile.am |   1 +
 45 files changed, 1668 insertions(+), 1421 deletions(-)
 create mode 100644 m4/virt-linker-no-undefined.m4
 delete mode 100644 src/storage/storage_source.c
 delete mode 100644 src/storage/storage_source.h
 create mode 100644 src/util/virstoragefilebackend.c
 create mode 100644 src/util/virstoragefilebackend.h

-- 
2.14.3

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

Re: [libvirt] [PATCH] docs: document requirement to provide Signed-off-by lines for DCO

[Kashyap Chamarthy]

On Mon, Jan 22, 2018 at 12:40:37PM +, Daniel P. Berrange wrote:
> Document that contributors are required to assert compliance with the
> Developers Certification of Origin 1.1, by providing Signed-off-by tags
> for all commit messages. The DCO is formally stating what we have long
> implicitly expected of contributors in terms of their legal rights to
> make the contribution. This puts the project in a stronger position
> should any questions around contributions be raised going forward in the
> future.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  docs/hacking.html.in | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>

With the nits Andrea pointed out fixed, FWIW:

Reviewed-by: Kashyap Chamarthy 

[...]

-- 
/kashyap

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

Re: [libvirt] with vhostuser I cannot use hugepages

[Michal Privoznik]

On 01/24/2018 10:19 PM, Mooney, Sean K wrote:
> Ah I see the error
> 
> You have
> 
> 
>   
>   
> 
> 
> Not
> 
> 
>   
> 
>   
> 

Well, even in the former case libvirt should pick up some hugepages
(with current implementation it usually picks 2M)

Michal

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

Re: [libvirt] [PATCH v3 0/3] nodedev: update caps before quering nodedev infos

[Erik Skultety]

On Wed, Jan 10, 2018 at 08:14:48PM +0800, Wu Zongyong wrote:
> Some capabilities of node devices rely on what driver they bound to,
> and therefore, these capabilities may change when the driver change.
> In current implemention, it is not consistent between real status and
> the status we get by invoking nodedev interfaces.
> So, this series of patches try to manually update devices' capabilities
> each time before nodedev driver interfaces invoked.

Hi, so I finally posted the series I mentioned in my previous reviews,
unfortunately, after applying your patches there were a lot of merge conflicts
so I had to write them from scratch, so I at least added you as an honorable
mention in patch equivalents of your patches 2 and 3. Unless someone objects to
the series as a whole, your patch 1 will be unnecessary.

Feel free to have a look at the series [1] and test whether it works as
expected (it did in my testing, both virsh and python).

Erik

[1] https://www.redhat.com/archives/libvir-list/2018-January/msg00851.html

>
> Wu Zongyong (3):
>
>   v2->v3:
> 1. split a single patch to three part
> 2. fix memory leak and lock problems
> 3. update caps before invoking nodedev driver interfaces (refer to patch 
> 3/3)
>
>
>   nodedev: add macro guard to node_device_udev header file
>   nodedev: update mdev_types caps before dumpxml
>   nodedev: update caps before invoking nodedev driver interfaces
>
>  src/node_device/node_device_driver.c  | 55 
> +++
>  src/node_device/node_device_linux_sysfs.c | 22 +
>  src/node_device/node_device_udev.c| 37 +
>  src/node_device/node_device_udev.h| 18 +++---
>  4 files changed, 121 insertions(+), 11 deletions(-)
>
> --
> 1.9.1
>

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

Re: [libvirt] with vhostuser I cannot use hugepages

[Michal Privoznik]

On 01/24/2018 07:53 PM, Adnan Mundres wrote:
> In my setup I am using qemu-kvm without openstack. I am trying to use ovs 
> with dpdk. In my xml file I added following lines for the dpdkvhostuser
>            memory="16777216" unit='KiB' memAccess="shared"/>       I have 
> enabled hugepages during boot time> ot@mvmgptb11hyp01 hyp-1]# cat 
> /proc/meminfo | grep -i
hugeAnonHugePages: 126976 kBHugePages_Total: 100HugePages_Free:
80HugePages_Rsvd: 0HugePages_Surp: 0Hugepagesize: 1048576 kB
> But When I start my vm (virsh start vm1.xml) I am seeing that this vm is not 
> using memory from hugepages, rather it is taking memory from total memory. 
> When I checked the log file I see that it is using
> -object 
> memory-backend-file,id=ram-node0,prealloc=yes,mem-path=/var/lib/libvirt/qemu/ram,share=yes
> It should use backend memory as following
> -object 
> memory-backend-file,id=ram-node0,prealloc=yes,mem-path=/dev/hugepages/libvirt/qemu/vm1,share=yes
> Any idea how can I use memory from hugepages

What version of libvirt are you using? There were some fixes made
recently. Try latest git HEAD and if the issue still persists please
file a bug report attaching domain XML and debug logs. I'm unable to
reproduce with the current git.

Michal

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

[libvirt] [PATCH 15/15] conf: nodedev: Update PCI mdev capabilities dynamically

[Erik Skultety]

Just like SRIOV, a PCI device is only capable of the mediated devices
framework when it's bound to the vendor native driver, thus if a driver
change occurs, e.g. vendor_native->vfio, we need to refresh some of the
device's capabilities to reflect the reality, mdev included.

Signed-off-by: Erik Skultety 
Suggested-by: Wu Zongyong 
---
 src/conf/node_device_conf.c| 34 +++---
 src/node_device/node_device_udev.c |  1 -
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5fc5f6708..9e4273855 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2758,6 +2758,34 @@ virNodeDeviceGetPCIIOMMUGroupCaps(virNodeDevCapPCIDevPtr 
pci_dev)
 }
 
 
+static int
+virNodeDeviceGetPCIMdevTypesCaps(const char *sysfspath,
+ virNodeDevCapPCIDevPtr pci_dev)
+{
+virMediatedDeviceTypePtr *types = NULL;
+int rc = 0;
+size_t i;
+
+/* this could be a refresh, so clear out the old data */
+for (i = 0; i < pci_dev->nmdev_types; i++)
+   virMediatedDeviceTypeFree(pci_dev->mdev_types[i]);
+VIR_FREE(pci_dev->mdev_types);
+pci_dev->nmdev_types = 0;
+pci_dev->flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_MDEV;
+
+rc = virPCIGetMdevTypes(sysfspath, );
+
+if (rc <= 0)
+return rc;
+
+VIR_STEAL_PTR(pci_dev->mdev_types, types);
+pci_dev->nmdev_types = rc;
+pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_MDEV;
+
+return 0;
+}
+
+
 /* virNodeDeviceGetPCIDynamicCaps() get info that is stored in sysfs
  * about devices related to this device, i.e. things that can change
  * without this device itself changing. These must be refreshed
@@ -2768,9 +2796,9 @@ int
 virNodeDeviceGetPCIDynamicCaps(const char *sysfsPath,
virNodeDevCapPCIDevPtr pci_dev)
 {
-if (virNodeDeviceGetPCISRIOVCaps(sysfsPath, pci_dev) < 0)
-return -1;
-if (virNodeDeviceGetPCIIOMMUGroupCaps(pci_dev) < 0)
+if (virNodeDeviceGetPCISRIOVCaps(sysfsPath, pci_dev) < 0 ||
+virNodeDeviceGetPCIIOMMUGroupCaps(pci_dev) < 0 ||
+virNodeDeviceGetPCIMdevTypesCaps(sysfsPath, pci_dev) < 0)
 return -1;
 return 0;
 }
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 1ccf1f8b4..e10660ba0 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -484,7 +484,6 @@ udevProcessPCI(struct udev_device *device,
 }
 
 ret = 0;
-
  cleanup:
 virPCIDeviceFree(pciDev);
 virPCIEDeviceInfoFree(pci_express);
-- 
2.13.6

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

[libvirt] [PATCH 14/15] conf: Replace usage of virNodeDevCapMdevType with virMediatedDeviceType

[Erik Skultety]

Now that we have all the building blocks in place, switch the nodedev
driver to use the "new" virMediatedDeviceType type instead of the "old"
virNodeDevCapMdevType one.

Signed-off-by: Erik Skultety 
---
 src/conf/node_device_conf.c | 21 -
 src/conf/node_device_conf.h | 14 +-
 src/libvirt_private.syms|  1 -
 3 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 3aefc9e5b..5fc5f6708 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -92,19 +92,6 @@ virNodeDevCapsDefParseString(const char *xpath,
 
 
 void
-virNodeDevCapMdevTypeFree(virNodeDevCapMdevTypePtr type)
-{
-if (!type)
-return;
-
-VIR_FREE(type->id);
-VIR_FREE(type->name);
-VIR_FREE(type->device_api);
-VIR_FREE(type);
-}
-
-
-void
 virNodeDeviceDefFree(virNodeDeviceDefPtr def)
 {
 virNodeDevCapsDefPtr caps;
@@ -285,7 +272,7 @@ virNodeDeviceCapPCIDefFormat(virBufferPtr buf,
 virBufferAddLit(buf, "\n");
 virBufferAdjustIndent(buf, 2);
 for (i = 0; i < data->pci_dev.nmdev_types; i++) {
-virNodeDevCapMdevTypePtr type = data->pci_dev.mdev_types[i];
+virMediatedDeviceTypePtr type = data->pci_dev.mdev_types[i];
 virBufferEscapeString(buf, "\n", type->id);
 virBufferAdjustIndent(buf, 2);
 if (type->name)
@@ -1546,7 +1533,7 @@ virNodeDevPCICapMdevTypesParseXML(xmlXPathContextPtr ctxt,
 xmlNodePtr orignode = NULL;
 xmlNodePtr *nodes = NULL;
 int nmdev_types = -1;
-virNodeDevCapMdevTypePtr type = NULL;
+virMediatedDeviceTypePtr type = NULL;
 size_t i;
 
 if ((nmdev_types = virXPathNodeSet("./type", ctxt, )) < 0)
@@ -1593,7 +1580,7 @@ virNodeDevPCICapMdevTypesParseXML(xmlXPathContextPtr ctxt,
 ret = 0;
  cleanup:
 VIR_FREE(nodes);
-virNodeDevCapMdevTypeFree(type);
+virMediatedDeviceTypeFree(type);
 ctxt->node = orignode;
 return ret;
 }
@@ -2176,7 +2163,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
 VIR_FREE(data->pci_dev.iommuGroupDevices);
 virPCIEDeviceInfoFree(data->pci_dev.pci_express);
 for (i = 0; i < data->pci_dev.nmdev_types; i++)
-virNodeDevCapMdevTypeFree(data->pci_dev.mdev_types[i]);
+virMediatedDeviceTypeFree(data->pci_dev.mdev_types[i]);
 VIR_FREE(data->pci_dev.mdev_types);
 break;
 case VIR_NODE_DEV_CAP_USB_DEV:
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 53cdfdb01..685ae3034 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -139,15 +139,6 @@ struct _virNodeDevCapSystem {
 virNodeDevCapSystemFirmware firmware;
 };
 
-typedef struct _virNodeDevCapMdevType virNodeDevCapMdevType;
-typedef virNodeDevCapMdevType *virNodeDevCapMdevTypePtr;
-struct _virNodeDevCapMdevType {
-char *id;
-char *name;
-char *device_api;
-unsigned int available_instances;
-};
-
 typedef struct _virNodeDevCapMdev virNodeDevCapMdev;
 typedef virNodeDevCapMdev *virNodeDevCapMdevPtr;
 struct _virNodeDevCapMdev {
@@ -178,7 +169,7 @@ struct _virNodeDevCapPCIDev {
 int numa_node;
 virPCIEDeviceInfoPtr pci_express;
 int hdrType; /* enum virPCIHeaderType or -1 */
-virNodeDevCapMdevTypePtr *mdev_types;
+virMediatedDeviceTypePtr *mdev_types;
 size_t nmdev_types;
 };
 
@@ -358,9 +349,6 @@ virNodeDeviceDefFree(virNodeDeviceDefPtr def);
 void
 virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps);
 
-void
-virNodeDevCapMdevTypeFree(virNodeDevCapMdevTypePtr type);
-
 # define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP \
 (VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM| \
  VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV   | \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8d4c8dd3f..2e20304ad 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -695,7 +695,6 @@ virNetDevIPRouteParseXML;
 
 
 # conf/node_device_conf.h
-virNodeDevCapMdevTypeFree;
 virNodeDevCapsDefFree;
 virNodeDevCapTypeFromString;
 virNodeDevCapTypeToString;
-- 
2.13.6

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

[libvirt] [PATCH 13/15] nodedev: udev: Drop the unused mdev type helpers

[Erik Skultety]

These are not necessary anymore, since these are going to be shadowed by
the helpers provided by util/virmdev.c module.

Signed-off-by: Erik Skultety 
---
 src/node_device/node_device_udev.c | 119 -
 1 file changed, 119 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index 519b0bf6f..1ccf1f8b4 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -393,119 +393,6 @@ udevTranslatePCIIds(unsigned int vendor,
 
 
 static int
-udevFillMdevType(struct udev_device *device,
- const char *dir,
- virNodeDevCapMdevTypePtr type)
-{
-int ret = -1;
-char *attrpath = NULL;
-
-#define MDEV_GET_SYSFS_ATTR(attr_name, cb, ...) \
-do { \
-if (virAsprintf(, "%s/%s", dir, #attr_name) < 0) \
-goto cleanup; \
- \
-if (cb(device, attrpath, __VA_ARGS__) < 0) \
-goto cleanup; \
- \
-VIR_FREE(attrpath); \
-} while (0) \
-
-if (VIR_STRDUP(type->id, last_component(dir)) < 0)
-goto cleanup;
-
-/* query udev for the attributes under subdirectories using the relative
- * path stored in @dir, i.e. 'mdev_supported_types/'
- */
-MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr, >name);
-MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr, >device_api);
-MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr,
->available_instances, 10);
-
-#undef MDEV_GET_SYSFS_ATTR
-
-ret = 0;
- cleanup:
-VIR_FREE(attrpath);
-return ret;
-}
-
-
-static int
-udevPCIGetMdevTypesCap(struct udev_device *device,
-   virNodeDevCapPCIDevPtr pcidata)
-{
-int ret = -1;
-int dirret = -1;
-DIR *dir = NULL;
-struct dirent *entry;
-char *path = NULL;
-char *tmppath = NULL;
-virNodeDevCapMdevTypePtr type = NULL;
-virNodeDevCapMdevTypePtr *types = NULL;
-size_t ntypes = 0;
-size_t i;
-
-if (virAsprintf(, "%s/mdev_supported_types",
-udev_device_get_syspath(device)) < 0)
-return -1;
-
-if ((dirret = virDirOpenIfExists(, path)) < 0)
-goto cleanup;
-
-if (dirret == 0) {
-ret = 0;
-goto cleanup;
-}
-
-if (VIR_ALLOC(types) < 0)
-goto cleanup;
-
-/* UDEV doesn't report attributes under subdirectories by default but is
- * able to query them if the path to the attribute is relative to the
- * device's base path, e.g. /sys/devices/../:00:01.0/ is the device's
- * base path as udev reports it, but we're interested in attributes under
- * /sys/devices/../:00:01.0/mdev_supported_types//. So, we need 
to
- * scan the subdirectories ourselves.
- */
-while ((dirret = virDirRead(dir, , path)) > 0) {
-if (VIR_ALLOC(type) < 0)
-goto cleanup;
-
-/* construct the relative mdev type path bit for udev */
-if (virAsprintf(, "mdev_supported_types/%s", entry->d_name) < 
0)
-goto cleanup;
-
-if (udevFillMdevType(device, tmppath, type) < 0)
-goto cleanup;
-
-if (VIR_APPEND_ELEMENT(types, ntypes, type) < 0)
-goto cleanup;
-
-VIR_FREE(tmppath);
-}
-
-if (dirret < 0)
-goto cleanup;
-
-VIR_STEAL_PTR(pcidata->mdev_types, types);
-pcidata->nmdev_types = ntypes;
-pcidata->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_MDEV;
-ntypes = 0;
-ret = 0;
- cleanup:
-virNodeDevCapMdevTypeFree(type);
-for (i = 0; i < ntypes; i++)
-virNodeDevCapMdevTypeFree(types[i]);
-VIR_FREE(types);
-VIR_FREE(path);
-VIR_FREE(tmppath);
-VIR_DIR_CLOSE(dir);
-return ret;
-}
-
-
-static int
 udevProcessPCI(struct udev_device *device,
virNodeDeviceDefPtr def)
 {
@@ -596,12 +483,6 @@ udevProcessPCI(struct udev_device *device,
 }
 }
 
-/* check whether the device is mediated devices framework capable, if so,
- * process it
- */
-if (udevPCIGetMdevTypesCap(device, pci_dev) < 0)
-goto cleanup;
-
 ret = 0;
 
  cleanup:
-- 
2.13.6

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

[libvirt] [PATCH 12/15] util: pci: Introduce virPCIGetMdevTypes helper

[Erik Skultety]

This is a replacement for the existing udevPCIGetMdevTypesCap which is
static to the udev backend. This simple helper constructs the sysfs path
from the device's base path for each mdev type and queries the
corresponding attributes of that type.

Signed-off-by: Erik Skultety 
---
 src/libvirt_private.syms |  1 +
 src/util/virpci.c| 58 
 src/util/virpci.h|  4 
 3 files changed, 63 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 75eaf1d4c..8d4c8dd3f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2456,6 +2456,7 @@ virPCIDeviceWaitForCleanup;
 virPCIEDeviceInfoFree;
 virPCIGetDeviceAddressFromSysfsLink;
 virPCIGetHeaderType;
+virPCIGetMdevTypes;
 virPCIGetNetName;
 virPCIGetPhysicalFunction;
 virPCIGetVirtualFunctionIndex;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index fe57bef32..12d7ef0e4 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -3027,6 +3027,64 @@ virPCIGetVirtualFunctionInfo(const char 
*vf_sysfs_device_path,
 return ret;
 }
 
+
+int
+virPCIGetMdevTypes(const char *sysfspath,
+   virMediatedDeviceTypePtr **types)
+{
+int ret = -1;
+int dirret = -1;
+DIR *dir = NULL;
+struct dirent *entry;
+char *types_path = NULL;
+char *tmppath = NULL;
+virMediatedDeviceTypePtr mdev_type = NULL;
+virMediatedDeviceTypePtr *mdev_types = NULL;
+size_t ntypes = 0;
+size_t i;
+
+if (virAsprintf(_path, "%s/mdev_supported_types", sysfspath) < 0)
+return -1;
+
+if ((dirret = virDirOpenIfExists(, types_path)) < 0)
+goto cleanup;
+
+if (dirret == 0) {
+ret = 0;
+goto cleanup;
+}
+
+while ((dirret = virDirRead(dir, , types_path)) > 0) {
+/* append the type id to the path and read the attributes from there */
+if (virAsprintf(, "%s/%s", types_path, entry->d_name) < 0)
+goto cleanup;
+
+if (virMediatedDeviceTypeReadAttrs(tmppath, _type) < 0)
+goto cleanup;
+
+if (VIR_APPEND_ELEMENT(mdev_types, ntypes, mdev_type) < 0)
+goto cleanup;
+
+VIR_FREE(tmppath);
+}
+
+if (dirret < 0)
+goto cleanup;
+
+VIR_STEAL_PTR(*types, mdev_types);
+ret = ntypes;
+ntypes = 0;
+ cleanup:
+virMediatedDeviceTypeFree(mdev_type);
+for (i = 0; i < ntypes; i++)
+virMediatedDeviceTypeFree(mdev_types[i]);
+VIR_FREE(mdev_types);
+VIR_FREE(types_path);
+VIR_FREE(tmppath);
+VIR_DIR_CLOSE(dir);
+return ret;
+}
+
 #else
 static const char *unsupported = N_("not supported on non-linux platforms");
 
diff --git a/src/util/virpci.h b/src/util/virpci.h
index f1fbe39e6..a0bc0a474 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -25,6 +25,7 @@
 # define __VIR_PCI_H__
 
 # include "internal.h"
+# include "virmdev.h"
 # include "virobject.h"
 # include "virutil.h"
 
@@ -249,4 +250,7 @@ int virPCIGetHeaderType(virPCIDevicePtr dev, int *hdrType);
 
 void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev);
 
+int virPCIGetMdevTypes(const char *sysfspath,
+   virMediatedDeviceType ***types);
+
 #endif /* __VIR_PCI_H__ */
-- 
2.13.6

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

[libvirt] [PATCH 11/15] util: mdev: Introduce virMediatedDeviceTypeReadAttrs getter

[Erik Skultety]

This should serve as a replacement for the existing udevFillMdevType
which is responsible for fetching the device type's attributes from the
sysfs interface. The problem with the existing solution is that it's
tied to the udev backend.

Signed-off-by: Erik Skultety 
---
 src/util/virmdev.c | 34 ++
 src/util/virmdev.h |  5 +
 2 files changed, 39 insertions(+)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index db679b8a6..b57cc3ed9 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -496,3 +496,37 @@ virMediatedDeviceTypeFree(virMediatedDeviceTypePtr type)
 VIR_FREE(type->device_api);
 VIR_FREE(type);
 }
+
+
+int
+virMediatedDeviceTypeReadAttrs(const char *sysfspath,
+   virMediatedDeviceTypePtr *type)
+{
+int ret = -1;
+virMediatedDeviceTypePtr tmp = NULL;
+
+#define MDEV_GET_SYSFS_ATTR(attr, dst, cb) \
+do { \
+if (cb(dst, "%s/%s", sysfspath, attr) < 0) \
+goto cleanup; \
+} while (0) \
+
+if (VIR_ALLOC(tmp) < 0)
+goto cleanup;
+
+if (VIR_STRDUP(tmp->id, last_component(sysfspath)) < 0)
+goto cleanup;
+
+MDEV_GET_SYSFS_ATTR("name", >name, virFileReadValueString);
+MDEV_GET_SYSFS_ATTR("device_api", >device_api, 
virFileReadValueString);
+MDEV_GET_SYSFS_ATTR("available_instances", >available_instances,
+virFileReadValueUint);
+
+#undef MDEV_GET_SYSFS_ATTR
+
+VIR_STEAL_PTR(*type, tmp);
+ret = 0;
+ cleanup:
+virMediatedDeviceTypeFree(tmp);
+return ret;
+}
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 320610ab9..01ab02e75 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -129,4 +129,9 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr 
dst,
 
 void
 virMediatedDeviceTypeFree(virMediatedDeviceTypePtr type);
+
+int
+virMediatedDeviceTypeReadAttrs(const char *sysfspath,
+   virMediatedDeviceTypePtr *type);
+
 #endif /* __VIR_MDEV_H__ */
-- 
2.13.6

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

[libvirt] [PATCH 08/15] conf: nodedev: Refresh capabilities before touching them

[Erik Skultety]

Most of them are static, however in case of PCI and SCSI_HOST devices,
the nested capabilities can change dynamically, e.g. due to a driver
change (from host_pci_driver -> vfio_pci).

Signed-off-by: Erik Skultety 
Suggested-by: Wu Zongyong 
---
 src/conf/node_device_conf.c | 3 +++
 src/conf/virnodedeviceobj.c | 4 
 2 files changed, 7 insertions(+)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 9467bb415..3aefc9e5b 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2515,6 +2515,9 @@ virNodeDeviceCapsListExport(virNodeDeviceDefPtr def,
 tmp[ncaps] = cap; \
 } while (0)
 
+if (virNodeDeviceUpdateCaps(def) < 0)
+goto cleanup;
+
 if (want_list && VIR_ALLOC_N(tmp, VIR_NODE_DEV_CAP_LAST - 1) < 0)
 goto cleanup;
 
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 5360df805..34f754454 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -809,6 +809,10 @@ static bool
 virNodeDeviceMatch(virNodeDeviceObjPtr obj,
unsigned int flags)
 {
+/* Refresh the capabilities first, e.g. due to a driver change */
+if (virNodeDeviceUpdateCaps(obj->def) < 0)
+return false;
+
 /* filter by cap type */
 if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) {
 if (!(MATCH(SYSTEM)||
-- 
2.13.6

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

[libvirt] [PATCH 07/15] nodedev: Introduce virNodeDeviceCapsListExport

[Erik Skultety]

Whether asking for a number of capabilities supported by a device or
listing them, it's handled essentially by a copy-paste code, so extract
the common stuff into this new helper which also updates all
capabilities just before touching them.

Signed-off-by: Erik Skultety 
---
 src/conf/node_device_conf.c  | 73 +++
 src/conf/node_device_conf.h  |  5 +++
 src/libvirt_private.syms |  1 +
 src/node_device/node_device_driver.c | 75 +---
 4 files changed, 97 insertions(+), 57 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 217673a56..9467bb415 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2487,6 +2487,79 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
 }
 
 
+/**
+ * virNodeDeviceCapsListExport:
+ * @def: node device definition
+ * @list: pointer to an array to store all supported capabilities by a device
+ *
+ * Takes the definition, scans through all the capabilities that the device
+ * supports (including the nested caps) and populates a newly allocated list
+ * with them. Caller is responsible for freeing the list.
+ * If NULL is passed to @list, only the number of caps will be returned.
+ *
+ * Returns the number of capabilities the device supports, -1 on error.
+ */
+int
+virNodeDeviceCapsListExport(virNodeDeviceDefPtr def,
+virNodeDevCapType **list)
+{
+virNodeDevCapsDefPtr caps = NULL;
+virNodeDevCapType *tmp = NULL;
+bool want_list = !!list;
+int ncaps = 0;
+int ret = -1;
+
+#define MAYBE_ADD_CAP(cap) \
+do { \
+if (want_list) \
+tmp[ncaps] = cap; \
+} while (0)
+
+if (want_list && VIR_ALLOC_N(tmp, VIR_NODE_DEV_CAP_LAST - 1) < 0)
+goto cleanup;
+
+for (caps = def->caps; caps; caps = caps->next) {
+unsigned int flags;
+
+MAYBE_ADD_CAP(caps->data.type);
+ncaps++;
+
+/* check nested caps for a given type as well */
+if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
+flags = caps->data.scsi_host.flags;
+
+if (flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
+MAYBE_ADD_CAP(VIR_NODE_DEV_CAP_FC_HOST);
+ncaps++;
+}
+
+if (flags  & VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS) {
+MAYBE_ADD_CAP(VIR_NODE_DEV_CAP_VPORTS);
+ncaps++;
+}
+}
+
+if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
+flags = caps->data.pci_dev.flags;
+
+if (flags & VIR_NODE_DEV_CAP_FLAG_PCI_MDEV) {
+MAYBE_ADD_CAP(VIR_NODE_DEV_CAP_MDEV_TYPES);
+ncaps++;
+}
+}
+}
+
+#undef MAYBE_ADD_CAP
+
+if (want_list)
+VIR_STEAL_PTR(*list, tmp);
+ret = ncaps;
+ cleanup:
+VIR_FREE(tmp);
+return ret;
+}
+
+
 #ifdef __linux__
 
 int
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 7e32f5c05..53cdfdb01 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -403,4 +403,9 @@ virNodeDeviceGetPCIDynamicCaps(const char *sysfsPath,
 
 int
 virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def);
+
+int
+virNodeDeviceCapsListExport(virNodeDeviceDefPtr def,
+virNodeDevCapType **list);
+
 #endif /* __VIR_NODE_DEVICE_CONF_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6098cf121..1698e6227 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -699,6 +699,7 @@ virNodeDevCapMdevTypeFree;
 virNodeDevCapsDefFree;
 virNodeDevCapTypeFromString;
 virNodeDevCapTypeToString;
+virNodeDeviceCapsListExport;
 virNodeDeviceCreateVport;
 virNodeDeviceDefFormat;
 virNodeDeviceDefFree;
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 48f45474c..8fb08742b 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -306,8 +306,6 @@ nodeDeviceNumOfCaps(virNodeDevicePtr device)
 {
 virNodeDeviceObjPtr obj;
 virNodeDeviceDefPtr def;
-virNodeDevCapsDefPtr caps;
-int ncaps = 0;
 int ret = -1;
 
 if (!(obj = nodeDeviceObjFindByName(device->name)))
@@ -317,27 +315,7 @@ nodeDeviceNumOfCaps(virNodeDevicePtr device)
 if (virNodeDeviceNumOfCapsEnsureACL(device->conn, def) < 0)
 goto cleanup;
 
-for (caps = def->caps; caps; caps = caps->next) {
-++ncaps;
-
-if (caps->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
-if (caps->data.scsi_host.flags &
-VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)
-ncaps++;
-
-if (caps->data.scsi_host.flags &
-VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)
-ncaps++;
-}
-if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
-if (caps->data.pci_dev.flags &
-

[libvirt] [PATCH 10/15] util: mdev: Introduce virMediatedDeviceType structure

[Erik Skultety]

This is later going to replace the existing virNodeDevCapMdevType, since:
1) it's going to couple related stuff in a single module
2) util is supposed to contain helpers that are widely accessible across
the whole repository.

Signed-off-by: Erik Skultety 
---
 src/libvirt_private.syms |  1 +
 src/util/virmdev.c   | 13 +
 src/util/virmdev.h   | 12 
 3 files changed, 26 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1698e6227..75eaf1d4c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2181,6 +2181,7 @@ virMediatedDeviceModelTypeFromString;
 virMediatedDeviceModelTypeToString;
 virMediatedDeviceNew;
 virMediatedDeviceSetUsedBy;
+virMediatedDeviceTypeFree;
 
 
 
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index a5f52d10f..db679b8a6 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -483,3 +483,16 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr 
dst,
 }
 goto cleanup;
 }
+
+
+void
+virMediatedDeviceTypeFree(virMediatedDeviceTypePtr type)
+{
+if (!type)
+return;
+
+VIR_FREE(type->id);
+VIR_FREE(type->name);
+VIR_FREE(type->device_api);
+VIR_FREE(type);
+}
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 84cbb1f2a..320610ab9 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -37,6 +37,15 @@ typedef virMediatedDevice *virMediatedDevicePtr;
 typedef struct _virMediatedDeviceList virMediatedDeviceList;
 typedef virMediatedDeviceList *virMediatedDeviceListPtr;
 
+typedef struct _virMediatedDeviceType virMediatedDeviceType;
+typedef virMediatedDeviceType *virMediatedDeviceTypePtr;
+struct _virMediatedDeviceType {
+char *id;
+char *name;
+char *device_api;
+unsigned int available_instances;
+};
+
 typedef int (*virMediatedDeviceCallback)(virMediatedDevicePtr dev,
  const char *path, void *opaque);
 
@@ -117,4 +126,7 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr 
dst,
  virMediatedDeviceListPtr src,
  const char *drvname,
  const char *domname);
+
+void
+virMediatedDeviceTypeFree(virMediatedDeviceTypePtr type);
 #endif /* __VIR_MDEV_H__ */
-- 
2.13.6

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

[libvirt] [PATCH 09/15] util: mdev: Drop some unused symbols/includes from the header

[Erik Skultety]

There were some leftovers from early development which never got used.

Signed-off-by: Erik Skultety 
---
 src/util/virmdev.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 0b8e830f4..84cbb1f2a 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -22,7 +22,6 @@
 # include "internal.h"
 # include "virobject.h"
 # include "virutil.h"
-# include "virpci.h"
 
 typedef enum {
 VIR_MDEV_MODEL_TYPE_VFIO_PCI = 0,
@@ -35,8 +34,6 @@ VIR_ENUM_DECL(virMediatedDeviceModel)
 
 typedef struct _virMediatedDevice virMediatedDevice;
 typedef virMediatedDevice *virMediatedDevicePtr;
-typedef struct _virMediatedDeviceAddress virMediatedDeviceAddress;
-typedef virMediatedDeviceAddress *virMediatedDeviceAddressPtr;
 typedef struct _virMediatedDeviceList virMediatedDeviceList;
 typedef virMediatedDeviceList *virMediatedDeviceListPtr;
 
-- 
2.13.6

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

[libvirt] [PATCH 06/15] nodedev: Export nodeDeviceUpdateCaps from node_device_conf.c

[Erik Skultety]

Since we moved the helpers from nodedev driver to src/conf, the actual
'update' function using those helpers should be moved as well so that we
don't need to call back into the driver.

Signed-off-by: Erik Skultety 
---
 src/conf/node_device_conf.c  | 54 
 src/conf/node_device_conf.h  |  3 ++
 src/libvirt_private.syms |  1 +
 src/node_device/node_device_driver.c | 54 +---
 4 files changed, 59 insertions(+), 53 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5b0af559a..217673a56 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2433,6 +2433,60 @@ virNodeDeviceDeleteVport(virConnectPtr conn,
 }
 
 
+int
+virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
+{
+virNodeDevCapsDefPtr cap = def->caps;
+
+while (cap) {
+switch (cap->data.type) {
+case VIR_NODE_DEV_CAP_SCSI_HOST:
+virNodeDeviceGetSCSIHostCaps(>data.scsi_host);
+break;
+case VIR_NODE_DEV_CAP_SCSI_TARGET:
+virNodeDeviceGetSCSITargetCaps(def->sysfs_path,
+   >data.scsi_target);
+break;
+case VIR_NODE_DEV_CAP_NET:
+if (virNetDevGetLinkInfo(cap->data.net.ifname,
+ >data.net.lnk) < 0)
+return -1;
+virBitmapFree(cap->data.net.features);
+if (virNetDevGetFeatures(cap->data.net.ifname,
+ >data.net.features) < 0)
+return -1;
+break;
+case VIR_NODE_DEV_CAP_PCI_DEV:
+if (virNodeDeviceGetPCIDynamicCaps(def->sysfs_path,
+   >data.pci_dev) < 0)
+return -1;
+break;
+
+/* all types that (supposedly) don't require any updates
+ * relative to what's in the cache.
+ */
+case VIR_NODE_DEV_CAP_DRM:
+case VIR_NODE_DEV_CAP_SYSTEM:
+case VIR_NODE_DEV_CAP_USB_DEV:
+case VIR_NODE_DEV_CAP_USB_INTERFACE:
+case VIR_NODE_DEV_CAP_SCSI:
+case VIR_NODE_DEV_CAP_STORAGE:
+case VIR_NODE_DEV_CAP_FC_HOST:
+case VIR_NODE_DEV_CAP_VPORTS:
+case VIR_NODE_DEV_CAP_SCSI_GENERIC:
+case VIR_NODE_DEV_CAP_MDEV_TYPES:
+case VIR_NODE_DEV_CAP_MDEV:
+case VIR_NODE_DEV_CAP_CCW_DEV:
+case VIR_NODE_DEV_CAP_LAST:
+break;
+}
+cap = cap->next;
+}
+
+return 0;
+}
+
+
 #ifdef __linux__
 
 int
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 4e3154875..7e32f5c05 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -400,4 +400,7 @@ virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
 int
 virNodeDeviceGetPCIDynamicCaps(const char *sysfsPath,
virNodeDevCapPCIDevPtr pci_dev);
+
+int
+virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def);
 #endif /* __VIR_NODE_DEVICE_CONF_H__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0cd8086a6..6098cf121 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -711,6 +711,7 @@ virNodeDeviceGetPCIDynamicCaps;
 virNodeDeviceGetSCSIHostCaps;
 virNodeDeviceGetSCSITargetCaps;
 virNodeDeviceGetWWNs;
+virNodeDeviceUpdateCaps;
 
 
 # conf/node_device_event.h
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 2e42d3527..48f45474c 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -47,58 +47,6 @@
 virNodeDeviceDriverStatePtr driver;
 
 
-static int
-nodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
-{
-virNodeDevCapsDefPtr cap = def->caps;
-
-while (cap) {
-switch (cap->data.type) {
-case VIR_NODE_DEV_CAP_SCSI_HOST:
-virNodeDeviceGetSCSIHostCaps(>data.scsi_host);
-break;
-case VIR_NODE_DEV_CAP_SCSI_TARGET:
-virNodeDeviceGetSCSITargetCaps(def->sysfs_path,
- >data.scsi_target);
-break;
-case VIR_NODE_DEV_CAP_NET:
-if (virNetDevGetLinkInfo(cap->data.net.ifname, >data.net.lnk) 
< 0)
-return -1;
-virBitmapFree(cap->data.net.features);
-if (virNetDevGetFeatures(cap->data.net.ifname, 
>data.net.features) < 0)
-return -1;
-break;
-case VIR_NODE_DEV_CAP_PCI_DEV:
-if (virNodeDeviceGetPCIDynamicCaps(def->sysfs_path,
-   >data.pci_dev) < 0)
-  return -1;
-   break;
-
-/* all types that (supposedly) don't require any updates
- * relative to what's in the cache.
- */
-case VIR_NODE_DEV_CAP_DRM:
-case VIR_NODE_DEV_CAP_SYSTEM:
-case 

[libvirt] [PATCH 02/15] conf: nodedev: Rename virNodeDeviceCapMatch to virNodeDevObjHasCap

[Erik Skultety]

We currently have 2 methods that do the capability matching. This should
be condensed to a single function and all the derivates should just call
into that using a proper type conversion.

Signed-off-by: Erik Skultety 
---
 src/conf/virnodedeviceobj.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index a4d38b3a1..ccad59a4b 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -55,6 +55,8 @@ static void virNodeDeviceObjDispose(void *opaque);
 static void virNodeDeviceObjListDispose(void *opaque);
 static bool virNodeDeviceObjHasCapStr(const virNodeDeviceObj *obj,
   const char *cap);
+static bool virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
+   int type);
 
 static int
 virNodeDeviceObjOnceInit(void)
@@ -683,8 +685,8 @@ virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr 
devs,
 
 
 static bool
-virNodeDeviceCapMatch(virNodeDeviceObjPtr obj,
-  int type)
+virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
+   int type)
 {
 virNodeDevCapsDefPtr cap = NULL;
 
@@ -850,7 +852,7 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs,
 
 
 #define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && 
\
- virNodeDeviceCapMatch(obj, VIR_NODE_DEV_CAP_ ## FLAG))
+ virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG))
 static bool
 virNodeDeviceMatch(virNodeDeviceObjPtr obj,
unsigned int flags)
-- 
2.13.6

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

[libvirt] [PATCH 00/15] Nodedev-mdev overhaul

[Erik Skultety]

TL;DR:
nodedev:
- contained a decent amount of redundant code handling the same thing,
  now it doesn't.
- only updated dynamic capabilities during dumpXML, now it does every
  time it touches them
mdev:
- didn't update mdev capabilities at all, now it does

This series combines some long-needed refactor changes to the nodedev driver
with some necessary mdev fixes based on Wu Zongyong's patch series [1].
There's a lot of simple code movement due to the fact that update of the device
capabilities is strictly bound to the nodedev driver. The problem with the
existing approach is that in order to properly update all capabilities,
especially mdev, we would have to violate the logical code flow we have and
call back into the driver to have access to sysfs, i.e. driver->(conf|obj)
handling->util_helpers->DRIVER. Therefore to resolve it, along with all the
compilation dependencies, I moved most of the capability handling out of the
driver into src/conf/node_device_conf.c which already contained more than just
parsing and formatting of the capabilities. I also had to move the existing
virNodeDevCapMdevType into src/util so that the util helpers would know the
type they're working with.

[1] https://www.redhat.com/archives/libvir-list/2018-January/msg00315.html

Erik Skultety (15):
  conf: nodedev: Rename virNodeDevObjHasCap to virNodeDevObjHasCapStr
  conf: nodedev: Rename virNodeDeviceCapMatch to virNodeDevObjHasCap
  conf: nodedev: Convert virNodeDevObjHasCapStr to a simple wrapper
  nodedev: Drop the nodeDeviceSysfsGetSCSIHostCaps wrapper
  nodedev: Move the sysfs-related cap handling to node_device_conf.c
  nodedev: Export nodeDeviceUpdateCaps from node_device_conf.c
  nodedev: Introduce virNodeDeviceCapsListExport
  conf: nodedev: Refresh capabilities before touching them
  util: mdev: Drop some unused symbols/includes from the header
  util: mdev: Introduce virMediatedDeviceType structure
  util: mdev: Introduce virMediatedDeviceTypeReadAttrs getter
  util: pci: Introduce virPCIGetMdevTypes helper
  nodedev: udev: Drop the unused mdev type helpers
  conf: Replace usage of virNodeDevCapMdevType with
virMediatedDeviceType
  conf: nodedev: Update PCI mdev capabilities dynamically

 src/Makefile.am   |   4 +-
 src/conf/node_device_conf.c   | 343 --
 src/conf/node_device_conf.h   |  29 +--
 src/conf/virnodedeviceobj.c   |  74 ++-
 src/libvirt_private.syms  |   7 +-
 src/node_device/node_device_driver.c  | 130 ++-
 src/node_device/node_device_hal.c |   5 +-
 src/node_device/node_device_linux_sysfs.c | 218 ---
 src/node_device/node_device_linux_sysfs.h |  34 ---
 src/node_device/node_device_udev.c| 127 +--
 src/util/virmdev.c|  47 
 src/util/virmdev.h|  20 +-
 src/util/virpci.c |  58 +
 src/util/virpci.h |   4 +
 14 files changed, 516 insertions(+), 584 deletions(-)
 delete mode 100644 src/node_device/node_device_linux_sysfs.c
 delete mode 100644 src/node_device/node_device_linux_sysfs.h

--
2.13.6

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

[libvirt] [PATCH 03/15] conf: nodedev: Convert virNodeDevObjHasCapStr to a simple wrapper

[Erik Skultety]

This patch drops the capability matching redundancy by simply converting
the string input to our internal types which are then in turn used for
the actual capability matching.

Signed-off-by: Erik Skultety 
---
 src/conf/virnodedeviceobj.c | 50 +
 1 file changed, 1 insertion(+), 49 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index ccad59a4b..5360df805 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -128,55 +128,7 @@ static bool
 virNodeDeviceObjHasCapStr(const virNodeDeviceObj *obj,
   const char *cap)
 {
-virNodeDevCapsDefPtr caps = obj->def->caps;
-const char *fc_host_cap =
-virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_FC_HOST);
-const char *vports_cap =
-virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_VPORTS);
-const char *mdev_types =
-virNodeDevCapTypeToString(VIR_NODE_DEV_CAP_MDEV_TYPES);
-
-while (caps) {
-if (STREQ(cap, virNodeDevCapTypeToString(caps->data.type))) {
-return true;
-} else {
-switch (caps->data.type) {
-case VIR_NODE_DEV_CAP_PCI_DEV:
-if ((STREQ(cap, mdev_types)) &&
-(caps->data.pci_dev.flags & 
VIR_NODE_DEV_CAP_FLAG_PCI_MDEV))
-return true;
-break;
-
-case VIR_NODE_DEV_CAP_SCSI_HOST:
-if ((STREQ(cap, fc_host_cap) &&
-(caps->data.scsi_host.flags & 
VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST)) ||
-(STREQ(cap, vports_cap) &&
-(caps->data.scsi_host.flags & 
VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS)))
-return true;
-break;
-
-case VIR_NODE_DEV_CAP_SYSTEM:
-case VIR_NODE_DEV_CAP_USB_DEV:
-case VIR_NODE_DEV_CAP_USB_INTERFACE:
-case VIR_NODE_DEV_CAP_NET:
-case VIR_NODE_DEV_CAP_SCSI_TARGET:
-case VIR_NODE_DEV_CAP_SCSI:
-case VIR_NODE_DEV_CAP_STORAGE:
-case VIR_NODE_DEV_CAP_FC_HOST:
-case VIR_NODE_DEV_CAP_VPORTS:
-case VIR_NODE_DEV_CAP_SCSI_GENERIC:
-case VIR_NODE_DEV_CAP_DRM:
-case VIR_NODE_DEV_CAP_MDEV_TYPES:
-case VIR_NODE_DEV_CAP_MDEV:
-case VIR_NODE_DEV_CAP_CCW_DEV:
-case VIR_NODE_DEV_CAP_LAST:
-break;
-}
-}
-
-caps = caps->next;
-}
-return false;
+return virNodeDeviceObjHasCap(obj, virNodeDevCapTypeFromString(cap));
 }
 
 
-- 
2.13.6

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

[libvirt] [PATCH 04/15] nodedev: Drop the nodeDeviceSysfsGetSCSIHostCaps wrapper

[Erik Skultety]

We can call directly the virNodeDeviceGetSCSIHostCaps helper instead.

Signed-off-by: Erik Skultety 
---
 src/conf/node_device_conf.c   | 12 
 src/node_device/node_device_driver.c  |  2 +-
 src/node_device/node_device_hal.c |  4 ++--
 src/node_device/node_device_linux_sysfs.c | 12 
 src/node_device/node_device_linux_sysfs.h |  1 -
 src/node_device/node_device_udev.c|  2 +-
 6 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index bf84fd2b3..70a753ebf 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2431,6 +2431,8 @@ virNodeDeviceDeleteVport(virConnectPtr conn,
 }
 
 
+#ifdef __linux__
+
 int
 virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host)
 {
@@ -2511,3 +2513,13 @@ virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr 
scsi_host)
 VIR_FREE(tmp);
 return ret;
 }
+
+#else
+
+int
+virNodeDeviceGetSCSIHostCaps(virNodeDevCap)
+{
+return -1;
+}
+
+#endif /* __linux__ */
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index 6216a6977..a2f687942 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -56,7 +56,7 @@ nodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
 while (cap) {
 switch (cap->data.type) {
 case VIR_NODE_DEV_CAP_SCSI_HOST:
-nodeDeviceSysfsGetSCSIHostCaps(>data.scsi_host);
+virNodeDeviceGetSCSIHostCaps(>data.scsi_host);
 break;
 case VIR_NODE_DEV_CAP_SCSI_TARGET:
 nodeDeviceSysfsGetSCSITargetCaps(def->sysfs_path,
diff --git a/src/node_device/node_device_hal.c 
b/src/node_device/node_device_hal.c
index c19e327c9..4c50f4613 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -151,7 +151,7 @@ gather_pci_cap(LibHalContext *ctx, const char *udi,
 ignore_value(virStrToLong_ui(p+1, , 16, >pci_dev.function));
 }
 
-if (nodeDeviceSysfsGetPCIRelatedDevCaps(sysfs_path, >pci_dev) < 0) {
+if (virNodeDeviceGetPCIDynamicCaps(sysfs_path, >pci_dev) < 0) {
 VIR_FREE(sysfs_path);
 return -1;
 }
@@ -237,7 +237,7 @@ gather_scsi_host_cap(LibHalContext *ctx, const char *udi,
 
 (void)get_int_prop(ctx, udi, "scsi_host.host", (int *)>scsi_host.host);
 
-retval = nodeDeviceSysfsGetSCSIHostCaps(>scsi_host);
+retval = virNodeDeviceGetSCSIHostCaps(>scsi_host);
 
 if (retval == -1)
 goto out;
diff --git a/src/node_device/node_device_linux_sysfs.c 
b/src/node_device/node_device_linux_sysfs.c
index 6f438e5f3..b3f80a555 100644
--- a/src/node_device/node_device_linux_sysfs.c
+++ b/src/node_device/node_device_linux_sysfs.c
@@ -45,12 +45,6 @@
 
 VIR_LOG_INIT("node_device.node_device_linux_sysfs");
 
-int
-nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host)
-{
-return virNodeDeviceGetSCSIHostCaps(scsi_host);
-}
-
 
 int
 nodeDeviceSysfsGetSCSITargetCaps(const char *sysfsPath,
@@ -196,12 +190,6 @@ nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath,
 
 #else
 
-int
-nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host 
ATTRIBUTE_UNUSED)
-{
-return -1;
-}
-
 int nodeDeviceSysfsGetSCSITargetCaps(const char *sysfsPath ATTRIBUTE_UNUSED,
  virNodeDevCapSCSITargetPtr scsi_target 
ATTRIBUTE_UNUSED)
 {
diff --git a/src/node_device/node_device_linux_sysfs.h 
b/src/node_device/node_device_linux_sysfs.h
index 12cfe6341..9392d6934 100644
--- a/src/node_device/node_device_linux_sysfs.h
+++ b/src/node_device/node_device_linux_sysfs.h
@@ -25,7 +25,6 @@
 
 # include "node_device_conf.h"
 
-int nodeDeviceSysfsGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host);
 int nodeDeviceSysfsGetSCSITargetCaps(const char *sysfsPath,
  virNodeDevCapSCSITargetPtr scsi_target);
 int nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath,
diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c
index e0fca6159..4cc531d2c 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -781,7 +781,7 @@ udevProcessSCSIHost(struct udev_device *device 
ATTRIBUTE_UNUSED,
 return -1;
 }
 
-nodeDeviceSysfsGetSCSIHostCaps(>caps->data.scsi_host);
+virNodeDeviceGetSCSIHostCaps(>caps->data.scsi_host);
 
 if (udevGenerateDeviceName(device, def, NULL) != 0)
 return -1;
-- 
2.13.6

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

[libvirt] [PATCH 01/15] conf: nodedev: Rename virNodeDevObjHasCap to virNodeDevObjHasCapStr

[Erik Skultety]

We currently have 2 methods that do the capability matching. This should
be condensed to a single function and all the derivates should just call
into that using a proper type conversion.

Signed-off-by: Erik Skultety 
---
 src/conf/virnodedeviceobj.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index c4e3a40d3..a4d38b3a1 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -53,6 +53,8 @@ static virClassPtr virNodeDeviceObjClass;
 static virClassPtr virNodeDeviceObjListClass;
 static void virNodeDeviceObjDispose(void *opaque);
 static void virNodeDeviceObjListDispose(void *opaque);
+static bool virNodeDeviceObjHasCapStr(const virNodeDeviceObj *obj,
+  const char *cap);
 
 static int
 virNodeDeviceObjOnceInit(void)
@@ -121,8 +123,8 @@ virNodeDeviceObjGetDef(virNodeDeviceObjPtr obj)
 
 
 static bool
-virNodeDeviceObjHasCap(const virNodeDeviceObj *obj,
-   const char *cap)
+virNodeDeviceObjHasCapStr(const virNodeDeviceObj *obj,
+  const char *cap)
 {
 virNodeDevCapsDefPtr caps = obj->def->caps;
 const char *fc_host_cap =
@@ -375,7 +377,7 @@ virNodeDeviceObjListFindByCapCallback(const void *payload,
 int want = 0;
 
 virObjectLock(obj);
-if (virNodeDeviceObjHasCap(obj, matchstr))
+if (virNodeDeviceObjHasCapStr(obj, matchstr))
 want = 1;
 virObjectUnlock(obj);
 return want;
@@ -750,7 +752,7 @@ virNodeDeviceObjListNumOfDevicesCallback(void *payload,
 virObjectLock(obj);
 def = obj->def;
 if ((!filter || filter(data->conn, def)) &&
-(!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr)))
+(!data->matchstr || virNodeDeviceObjHasCapStr(obj, data->matchstr)))
 data->count++;
 
 virObjectUnlock(obj);
@@ -805,7 +807,7 @@ virNodeDeviceObjListGetNamesCallback(void *payload,
 def = obj->def;
 
 if ((!filter || filter(data->conn, def)) &&
-(!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) {
+(!data->matchstr || virNodeDeviceObjHasCapStr(obj, data->matchstr))) {
 if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) {
 data->error = true;
 goto cleanup;
-- 
2.13.6

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

[libvirt] [PATCH 05/15] nodedev: Move the sysfs-related cap handling to node_device_conf.c

[Erik Skultety]

The capabilities are defined/parsed/formatted/queried from this module,
no reason for 'update' not being part of the module as well. This also
involves some module-specific prefix changes.
This patch also drops the node_device_linux_sysfs module from the repo
since:
a) it only contained the capability handlers we just moved
b) it's only linked with the driver (by design) and thus unreachable to
other modules
c) we touch sysfs across all the src/util modules so the module being
deleted hasn't been serving its original intention for some time already.

Signed-off-by: Erik Skultety 
---
 src/Makefile.am   |   4 +-
 src/conf/node_device_conf.c   | 154 +-
 src/conf/node_device_conf.h   |   7 +
 src/libvirt_private.syms  |   2 +
 src/node_device/node_device_driver.c  |   7 +-
 src/node_device/node_device_hal.c |   1 -
 src/node_device/node_device_linux_sysfs.c | 206 --
 src/node_device/node_device_linux_sysfs.h |  33 -
 src/node_device/node_device_udev.c|   5 +-
 9 files changed, 168 insertions(+), 251 deletions(-)
 delete mode 100644 src/node_device/node_device_linux_sysfs.c
 delete mode 100644 src/node_device/node_device_linux_sysfs.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 166c9a8e9..b8ecd8df8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1168,9 +1168,7 @@ ACCESS_DRIVER_POLKIT_POLICY = \
 
 NODE_DEVICE_DRIVER_SOURCES = \
node_device/node_device_driver.c \
-   node_device/node_device_driver.h \
-   node_device/node_device_linux_sysfs.c \
-   node_device/node_device_linux_sysfs.h
+   node_device/node_device_driver.h
 
 NODE_DEVICE_DRIVER_HAL_SOURCES = \
node_device/node_device_hal.c \
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 70a753ebf..5b0af559a 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -33,11 +33,13 @@
 #include "virstring.h"
 #include "node_device_conf.h"
 #include "device_conf.h"
+#include "dirname.h"
 #include "virxml.h"
 #include "virbuffer.h"
 #include "viruuid.h"
 #include "virrandom.h"
 #include "virlog.h"
+#include "virfcp.h"
 
 #define VIR_FROM_THIS VIR_FROM_NODEDEV
 
@@ -2514,10 +2516,160 @@ virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr 
scsi_host)
 return ret;
 }
 
+
+int
+virNodeDeviceGetSCSITargetCaps(const char *sysfsPath,
+   virNodeDevCapSCSITargetPtr scsi_target)
+{
+int ret = -1;
+char *dir = NULL, *rport = NULL;
+
+VIR_DEBUG("Checking if '%s' is an FC remote port", scsi_target->name);
+
+/* /sys/devices/[...]/host0/rport-0:0-0/target0:0:0 -> rport-0:0-0 */
+if (!(dir = mdir_name(sysfsPath)))
+return -1;
+
+if (VIR_STRDUP(rport, last_component(dir)) < 0)
+goto cleanup;
+
+if (!virFCIsCapableRport(rport))
+goto cleanup;
+
+VIR_FREE(scsi_target->rport);
+VIR_STEAL_PTR(scsi_target->rport, rport);
+
+if (virFCReadRportValue(scsi_target->rport, "port_name",
+_target->wwpn) < 0) {
+VIR_WARN("Failed to read port_name for '%s'", scsi_target->rport);
+goto cleanup;
+}
+
+scsi_target->flags |= VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
+ret = 0;
+
+ cleanup:
+if (ret < 0) {
+VIR_FREE(scsi_target->rport);
+VIR_FREE(scsi_target->wwpn);
+scsi_target->flags &= ~VIR_NODE_DEV_CAP_FLAG_FC_RPORT;
+}
+VIR_FREE(rport);
+VIR_FREE(dir);
+
+return ret;
+}
+
+
+static int
+virNodeDeviceGetPCISRIOVCaps(const char *sysfsPath,
+ virNodeDevCapPCIDevPtr pci_dev)
+{
+size_t i;
+int ret;
+
+/* this could be a refresh, so clear out the old data */
+for (i = 0; i < pci_dev->num_virtual_functions; i++)
+   VIR_FREE(pci_dev->virtual_functions[i]);
+VIR_FREE(pci_dev->virtual_functions);
+pci_dev->num_virtual_functions = 0;
+pci_dev->max_virtual_functions = 0;
+pci_dev->flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+pci_dev->flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
+
+ret = virPCIGetPhysicalFunction(sysfsPath,
+_dev->physical_function);
+if (ret < 0)
+goto cleanup;
+
+if (pci_dev->physical_function)
+pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
+
+ret = virPCIGetVirtualFunctions(sysfsPath, _dev->virtual_functions,
+_dev->num_virtual_functions,
+_dev->max_virtual_functions);
+if (ret < 0)
+goto cleanup;
+
+if (pci_dev->num_virtual_functions > 0 ||
+pci_dev->max_virtual_functions > 0)
+pci_dev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
+
+ cleanup:
+return ret;
+}
+
+
+static int

Re: [libvirt] [PATCH 06/10] conf: Add support for cputune/cachetune

[Pavel Hrdina]

On Wed, Jan 24, 2018 at 10:32:07PM +0100, Martin Kletzander wrote:
> On Wed, Jan 24, 2018 at 03:04:37PM +0100, Pavel Hrdina wrote:
> > On Tue, Jan 23, 2018 at 07:05:15PM +0100, Martin Kletzander wrote:
> > > More info in the documentation, this is basically the XML 
> > > parsing/formatting
> > > support, schemas, tests and documentation for the new cputune/cachetune 
> > > element
> > > that will get used by following patches.
> > > 
> > > Signed-off-by: Martin Kletzander 
> > > ---
> > >  docs/formatdomain.html.in  |  54 
> > >  docs/schemas/domaincommon.rng  |  32 +++
> > >  src/conf/domain_conf.c | 295 
> > > -
> > >  src/conf/domain_conf.h |  13 +
> > >  tests/genericxml2xmlindata/cachetune-cdp.xml   |  36 +++
> > >  .../cachetune-colliding-allocs.xml |  30 +++
> > >  .../cachetune-colliding-tunes.xml  |  32 +++
> > >  .../cachetune-colliding-types.xml  |  30 +++
> > >  tests/genericxml2xmlindata/cachetune-small.xml |  29 ++
> > >  tests/genericxml2xmlindata/cachetune.xml   |  33 +++
> > >  tests/genericxml2xmltest.c |  10 +
> > >  11 files changed, 592 insertions(+), 2 deletions(-)
> > >  create mode 100644 tests/genericxml2xmlindata/cachetune-cdp.xml
> > >  create mode 100644 
> > > tests/genericxml2xmlindata/cachetune-colliding-allocs.xml
> > >  create mode 100644 
> > > tests/genericxml2xmlindata/cachetune-colliding-tunes.xml
> > >  create mode 100644 
> > > tests/genericxml2xmlindata/cachetune-colliding-types.xml
> > >  create mode 100644 tests/genericxml2xmlindata/cachetune-small.xml
> > >  create mode 100644 tests/genericxml2xmlindata/cachetune.xml
> > > 
> > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > > index d272cc1ba698..7b4d9051a551 100644
> > > --- a/docs/formatdomain.html.in
> > > +++ b/docs/formatdomain.html.in
> > > @@ -689,6 +689,10 @@
> > >  iothread_quota-1/iothread_quota
> > >  vcpusched vcpus='0-4,^3' scheduler='fifo' priority='1'/
> > >  iothreadsched iothreads='2' scheduler='batch'/
> > > +cachetune vcpus='0-3'
> > > +  cache id='0' level='3' type='both' size='3' unit='MiB'/
> > > +  cache id='1' level='3' type='both' size='3' unit='MiB'/
> > > +/cachetune
> > >/cputune
> > >...
> > >  /domain
> > > @@ -834,6 +838,56 @@
> > >  Since 1.2.13
> > >
> > > 
> > > +  cachetuneSince 
> > > 4.1.0
> > > +  
> > > +Optional cachetune element can control allocations 
> > > for CPU
> > > +caches using the resctrl on the host. Whether or not is this 
> > > supported
> > > +can be gathered from capabilities where some limitations like 
> > > minimum
> > > +size and required granularity are reported as well.  The required
> > 
> > s/  / /
> > 
> > > +attribute vcpus specifies to which vCPUs this 
> > > allocation
> > > +applies. A vCPU can only be member of one cachetune 
> > > element
> > > +allocations. Supported subelements are:
> > > +
> > > +  cache
> > > +  
> > > +This element controls the allocation of CPU cache and has the
> > > +following attributes:
> > > +
> > > +  level
> > > +  
> > > +Host cache level from which to allocate.
> > > +  
> > > +  id
> > > +  
> > > +Host cache id from which to allocate.
> > > +  
> > > +  type
> > > +  
> > > +Type of allocation.  Can be code for code
> > 
> > s/  / /
> > 
> > > +(instructions), data for data or 
> > > both
> > > +for both code and data (unified).  Currently the 
> > > allocation can
> > 
> > s/  / /
> > 
> 
> Fixed all three.
> 
> /me wonders why people still insist on this.  Is someone editing these
>files with proportional font?  Do we have any other output than
>HTML?  I guess this will be yet another thing I have to get used to
>and surrender.

I personally don't care about the double space or single space, this
was just for consistency.

> > > +be done only with the same type as the host supports, 
> > > meaning
> > > +you cannot request both for host with CDP
> > > +(code/data prioritization) enabled.
> > > +  
> > > +  size
> > > +  
> > > +The size of the region to allocate. The value by default 
> > > is in
> > > +bytes, but the unit attribute can be used 
> > > to scale
> > > +the value.
> > > +  
> > > +  unit (optional)
> > > +  
> > > +If specified it is the unit such as KiB, MiB, GiB, or TiB
> > > +

[libvirt] [PATCH v3] docs: formatdomain: Document the CPU feature 'name' attribute

[Kashyap Chamarthy]

Currently, the CPU feature 'name' XML attribute, as in:

[...]

  IvyBridge
  Intel
  

[...]

isn't explicitly documented in formatdomain.html.

Document it now.

Signed-off-by: Kashyap Chamarthy 
---
v3:
 - Wrap text in paragraph; fix phrasing [John Ferlan]
v2:
 - Remove redundant line [Eduardo Habkost]

 docs/formatdomain.html.in | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d272cc1ba..fc0885420 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1454,6 +1454,21 @@
 
 Since 0.8.5 the policy
 attribute can be omitted and will default to require.
+
+ Individual CPU feature names are specified as part of the
+name attribute. For example, to explicitly specify
+the 'pcid' feature with Intel IvyBridge CPU model:
+
+
+
+...
+cpu match='exact'
+  model fallback='forbid'IvyBridge/model
+  vendorIntel/vendor
+  feature policy='require' name='pcid'/
+/cpu
+...
+
   
 
   cache
-- 
2.13.6

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

Re: [libvirt] [PATCH v2] docs: formatdomain: Document the CPU feature 'name' attribute

[Kashyap Chamarthy]

On Wed, Jan 24, 2018 at 03:31:31PM -0500, John Ferlan wrote:
> On 01/24/2018 10:25 AM, Kashyap Chamarthy wrote:

[...]

> >  docs/formatdomain.html.in | 14 ++
> >  1 file changed, 14 insertions(+)
> > 
> 
> When built, formatted, rendered, and read in a browser one gets in one
> long line:

[...]

> So I think wrapping your change with  ...  will at least make it
> look like a separate paragraph within the  element description.

Ah, good catch.  Fixed in v3.

[...]

> > +Individual CPU feature names can be specified as part of the
> 
> Since the name attribute is required, rather than "can be" or "should
> be" (as Eduardo suggested), I think perhaps "features names are
> specified using the required name attribute."

I went with your above wording in v3.

> or
> 
> "The required name attribute is used specify each desired
> CPU feature."
> 
> (because we state initially "The cpu element can contain zero or more
> elements...").
> 
> > +name attribute. For example, to explicitly specify
> 
> s/specify/require

I used the verb 'specify' to indicate that there is an _action_ to be
taken.  To my non-native ears: "to explicitly require" sounds slightly
odd when asking to take an action.

But I'll defer to your native tounge intuition.

> Thoughts?  I can make the adjustment before pushing if desired.

Thanks for the review.  Sending a v3; feel free to adjust it as you see
fit.

-- 
/kashyap

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

[libvirt] [PATCH-RUBY] The first argument passed should be the disk not the size.

[Marius Rieder]

Calling block_resize on a Libvirt::Domain object leads to a TypeError.
The reason seems to be that ruby-libvirt tries to convert the size
instead of the disk to a string to pass as the disk parameter.
---
 ext/libvirt/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/libvirt/domain.c b/ext/libvirt/domain.c
index 7812450..aff1842 100644
--- a/ext/libvirt/domain.c
+++ b/ext/libvirt/domain.c
@@ -2882,7 +2882,7 @@ static VALUE libvirt_domain_block_resize(int argc, VALUE 
*argv, VALUE d)
 ruby_libvirt_generate_call_nil(virDomainBlockResize,
ruby_libvirt_connect_get(d),
ruby_libvirt_domain_get(d),
-   StringValueCStr(size), NUM2ULL(size),
+   StringValueCStr(disk), NUM2ULL(size),
ruby_libvirt_value_to_uint(flags));
 }
 #endif
-- 
2.7.4

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