Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel
On 08/11/2015 03:28 AM, Moshe Levi wrote: -Original Message- From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of Laine Stump Sent: Tuesday, August 11, 2015 9:27 AM To: libvir-list@redhat.com Cc: Moshe Levi Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel On 08/08/2015 05:34 AM, Moshe Levi wrote: This patch add virNetDevGetGFeaturesSize to get the supported gfeature size from the kernel --- This is interesting/possibly useful, but it doesn't fix the crash that users are experiencing. Here is a patch that should fix the crash: https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html I would rather have that patch pushed before this one (which will mean rebasing and resolving some merge conflicts). Ok I will rebase once you patch is merged. Laine's patch is now pushed - I assume at least parts of this will be necessary since there are reports of different GFEATURE_SIZE values... ...snip... virNetDevSendEthtoolIoctl() logs an error message, but it looks like you want for an error to be swallowed here (just returning size = 0). If that's the case then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors, then every caller to it needs to log the error. This was comment by John Ferlan he preferred the method will return size 0 if not supported or error. My previous patch which I send to him directly and not the ML return -1 on failure. But thinking about this again I don't want to swallow if error occur. What do you think? I think my non ML response to you was more to the effect of what purpose is returning size = 0 and it certainly wasn't perfectly clear what size = 0 to the caller meant... Taken out of context regarding the changes you sent me, my comment was: Then virNetDevGetGFeaturesSize can return -1 or a size, but it's not clear whether size == 0 could be true. The code only checks if size == -1 to fail and spits out another VIR_DEBUG message (one would already be spit out on ioctl() failures, so that's duplicitous). So the question is - is it possible to return size == 0 and if so, I assume that wouldn't be good. and to be fair I was reading that code after driving 13 hours while moving ;-) I do agree with some of the changes Laine posted in his first pass at fixing some inconsistencies in the code in one large patch (which were requested to be split up). Some issues were not a result of your patches, but previous patches which were more or less reused. In the long run if more patches are added to clean things up - that'll be good. We move forward and learn from our mistakes. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel
-Original Message- From: John Ferlan [mailto:jfer...@redhat.com] Sent: Wednesday, August 12, 2015 12:01 AM To: Moshe Levi; Laine Stump; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel On 08/11/2015 03:28 AM, Moshe Levi wrote: -Original Message- From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of Laine Stump Sent: Tuesday, August 11, 2015 9:27 AM To: libvir-list@redhat.com Cc: Moshe Levi Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel On 08/08/2015 05:34 AM, Moshe Levi wrote: This patch add virNetDevGetGFeaturesSize to get the supported gfeature size from the kernel --- This is interesting/possibly useful, but it doesn't fix the crash that users are experiencing. Here is a patch that should fix the crash: https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html I would rather have that patch pushed before this one (which will mean rebasing and resolving some merge conflicts). Ok I will rebase once you patch is merged. Laine's patch is now pushed - I assume at least parts of this will be necessary since there are reports of different GFEATURE_SIZE values... Ok, Do you want me to rebase my patch on top on this http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c001700a03f67d7c4 and fixing all Laine comments or to wait for the cleanup patch you mention below? ...snip... virNetDevSendEthtoolIoctl() logs an error message, but it looks like you want for an error to be swallowed here (just returning size = 0). If that's the case then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors, then every caller to it needs to log the error. This was comment by John Ferlan he preferred the method will return size 0 if not supported or error. My previous patch which I send to him directly and not the ML return -1 on failure. But thinking about this again I don't want to swallow if error occur. What do you think? I think my non ML response to you was more to the effect of what purpose is returning size = 0 and it certainly wasn't perfectly clear what size = 0 to the caller meant... Taken out of context regarding the changes you sent me, my comment was: Then virNetDevGetGFeaturesSize can return -1 or a size, but it's not clear whether size == 0 could be true. The code only checks if size == -1 to fail and spits out another VIR_DEBUG message (one would already be spit out on ioctl() failures, so that's duplicitous). So the question is - is it possible to return size == 0 and if so, I assume that wouldn't be good. and to be fair I was reading that code after driving 13 hours while moving ;-) I do agree with some of the changes Laine posted in his first pass at fixing some inconsistencies in the code in one large patch (which were requested to be split up). Some issues were not a result of your patches, but previous patches which were more or less reused. In the long run if more patches are added to clean things up - that'll be good. We move forward and learn from our mistakes. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain: Fix crash if trying to live update disk serial
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass diskserial XML to UpdateDevice, and the original device
didn't have a serial block, libvirtd crashes trying to read the original
NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other
properties needed the change too.
---
src/conf/domain_conf.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk-*
value either, right?
ACK - with that adjustment...
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fd0450f..f1e02e3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5871,28 +5871,28 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr
disk,
CHECK_EQ(transient, transient, true);
-if (disk-serial STRNEQ(disk-serial, orig_disk-serial)) {
+if (disk-serial STRNEQ_NULLABLE(disk-serial, orig_disk-serial)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
serial);
return false;
}
-if (disk-wwn STRNEQ(disk-wwn, orig_disk-wwn)) {
+if (disk-wwn STRNEQ_NULLABLE(disk-wwn, orig_disk-wwn)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
wwn);
return false;
}
-if (disk-vendor STRNEQ(disk-vendor, orig_disk-vendor)) {
+if (disk-vendor STRNEQ_NULLABLE(disk-vendor, orig_disk-vendor)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
vendor);
return false;
}
-if (disk-product STRNEQ(disk-product, orig_disk-product)) {
+if (disk-product STRNEQ_NULLABLE(disk-product, orig_disk-product))
{
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
product);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain: Fix crash if trying to live update disk serial
On 08/11/2015 05:28 PM, Cole Robinson wrote: On 08/11/2015 05:25 PM, John Ferlan wrote: On 08/10/2015 07:33 PM, Cole Robinson wrote: If you pass diskserial XML to UpdateDevice, and the original device didn't have a serial block, libvirtd crashes trying to read the original NULL serial string. Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) Using STRNEQ_NULLABLE means you probably don't have to check the disk-* value either, right? ACK - with that adjustment... It would change the semantic a bit; if the orig disk has a serial, but not the XML passed to UpdateDevice, libvirt would now error where previously it wouldn't. It might make sense but I didn't want to change behavior - Cole OK - fair enough... right - time to quit for the day I guess ;-) ACK to what's on list John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain: Fix crash if trying to live update disk serial
On 08/11/2015 05:32 PM, John Ferlan wrote: On 08/11/2015 05:28 PM, Cole Robinson wrote: On 08/11/2015 05:25 PM, John Ferlan wrote: On 08/10/2015 07:33 PM, Cole Robinson wrote: If you pass diskserial XML to UpdateDevice, and the original device didn't have a serial block, libvirtd crashes trying to read the original NULL serial string. Use _NULLABLE string comparisons to avoid the crash. A couple other properties needed the change too. --- src/conf/domain_conf.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) Using STRNEQ_NULLABLE means you probably don't have to check the disk-* value either, right? ACK - with that adjustment... It would change the semantic a bit; if the orig disk has a serial, but not the XML passed to UpdateDevice, libvirt would now error where previously it wouldn't. It might make sense but I didn't want to change behavior - Cole OK - fair enough... right - time to quit for the day I guess ;-) ACK to what's on list Thanks, pushed now - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] domain: Fix crash if trying to live update disk serial
On 08/11/2015 05:25 PM, John Ferlan wrote:
On 08/10/2015 07:33 PM, Cole Robinson wrote:
If you pass diskserial XML to UpdateDevice, and the original device
didn't have a serial block, libvirtd crashes trying to read the original
NULL serial string.
Use _NULLABLE string comparisons to avoid the crash. A couple other
properties needed the change too.
---
src/conf/domain_conf.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
Using STRNEQ_NULLABLE means you probably don't have to check the disk-*
value either, right?
ACK - with that adjustment...
It would change the semantic a bit; if the orig disk has a serial, but not
the XML passed to UpdateDevice, libvirt would now error where previously it
wouldn't. It might make sense but I didn't want to change behavior
- Cole
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fd0450f..f1e02e3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5871,28 +5871,28 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr
disk,
CHECK_EQ(transient, transient, true);
-if (disk-serial STRNEQ(disk-serial, orig_disk-serial)) {
+if (disk-serial STRNEQ_NULLABLE(disk-serial, orig_disk-serial)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
serial);
return false;
}
-if (disk-wwn STRNEQ(disk-wwn, orig_disk-wwn)) {
+if (disk-wwn STRNEQ_NULLABLE(disk-wwn, orig_disk-wwn)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
wwn);
return false;
}
-if (disk-vendor STRNEQ(disk-vendor, orig_disk-vendor)) {
+if (disk-vendor STRNEQ_NULLABLE(disk-vendor, orig_disk-vendor)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
vendor);
return false;
}
-if (disk-product STRNEQ(disk-product, orig_disk-product)) {
+if (disk-product STRNEQ_NULLABLE(disk-product,
orig_disk-product)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_(cannot modify field '%s' of the disk),
product);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel
On 08/11/2015 05:24 PM, Moshe Levi wrote: -Original Message- From: John Ferlan [mailto:jfer...@redhat.com] Sent: Wednesday, August 12, 2015 12:01 AM To: Moshe Levi; Laine Stump; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel On 08/11/2015 03:28 AM, Moshe Levi wrote: -Original Message- From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of Laine Stump Sent: Tuesday, August 11, 2015 9:27 AM To: libvir-list@redhat.com Cc: Moshe Levi Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel On 08/08/2015 05:34 AM, Moshe Levi wrote: This patch add virNetDevGetGFeaturesSize to get the supported gfeature size from the kernel --- This is interesting/possibly useful, but it doesn't fix the crash that users are experiencing. Here is a patch that should fix the crash: https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html I would rather have that patch pushed before this one (which will mean rebasing and resolving some merge conflicts). Ok I will rebase once you patch is merged. Laine's patch is now pushed - I assume at least parts of this will be necessary since there are reports of different GFEATURE_SIZE values... Ok, Do you want me to rebase my patch on top on this http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c001700a03f67d7c4 and fixing all Laine comments or to wait for the cleanup patch you mention below? I would say fixing a bug is more important than cleanup... Unless you feel like taking the time and applying the changes Laine proposed in separate patches rather than one mega patch... If I have a few cycles I might try to do, but no guarantees ;-) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel
On 08/11/2015 05:24 PM, Moshe Levi wrote: Laine's patch is now pushed - I assume at least parts of this will be necessary since there are reports of different GFEATURE_SIZE values... Ok, Do you want me to rebase my patch on top on this http://libvirt.org/git/?p=libvirt.git;a=commit;h=bfaaa2b681018f3705bae17c001700a03f67d7c4 and fixing all Laine comments or to wait for the cleanup patch you mention below? I would say just rebase your patch on top of the simpler patch that I pushed, and resend it; leave cleaning up the other nits that I found until later (if even then). Parts of my cleanup had the effect of causing a failure return any time the ioctl returned an error, which I now see may not be the best course of action, so some of my proposed changes are suspect at best :-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 0/4] qemu: Allow PCI virtio on ARM virt machine
(Alex - I cc'ed you because I addressed a question or two your way down towards the bottom). On 08/11/2015 02:52 AM, Pavel Fedin wrote: Hello! The original patches to support pcie-root severely restricted what could plug into what because in real hardware you can't plug a PCI device into a PCIe slot (physically it doesn't work) But how do you know whether the device is PCI or PCIe ? I don't see anything like this in the code, i see that for example all network cards are PCI, which is, BTW, not true in the real world. Two years ago when I first added support for q35-based machinetypes and the first pcie controllers, I had less information than I do now. When I looked in the ouput of qemu-kvm -device ? I saw that each device listed the type of bus it connected to (PCI or ISA), and assumed that even though at the time qemu didn't differentiate between PCI and PCIe there, since the two things *are* different in the real world eventually they likely would. I wanted the libvirt code to be prepared for that eventuality. Of course every example device (except the PCIe controllers themselves) ends up with the flag saying that it can connect to a PCI bus, not PCIe). Later I was told that, unlike the real world where, if nothing else, the physical slots themselves limit you, any normal PCI device in qemu could be plugged into a PCI or PCIe slot. There still are several restrictions though, which showed themselves as more complicated than just the naive PCI vs PCIe that I originally imagined - just look at the restrictions on the different PCIe controllers: (pcie-sw-up-port == pcie-switch-upstream-port, pcie-sw-dn-port == pcie-switch-downstream-port) name upstream downstream -- --- pcie-rootnone any endpoint pcie-root-port dmi-to-pci-bridge pci-bridge 31 ports NO hotplug dmi-to-pci-bridgepcie-root any endpoint device pcie-root-port pcie-sw-up-port pcie-sw-dn-port NO hotplug 32 ports NO hotplug pcie-root-port pcie-root-only any endpoint NO hotplug dmi-to-pci-bridge pcie-sw-up-port 1 port hotpluggable pcie-sw-up-port pcie-root-port pcie-sw-dn-port pcie-sw-dn-port32 ports kind of hotpluggable kind of hotpluggable pcie-sw-dn-port pcie-sw-up-portany endpoint kind of hotplug pcie-sw-up-port 1 port hotpluggable pci-bridge pci-root any endpoint pcie-root pci-bridge dmi-to-pci-bridge 32 ports hotpluggable pcie-root-port pcie-sw-dn-port NO hotplug (now) So the original restrictions I placed on what could plugin where were *too* restrictive for endpoint devices, but other restrictions were useful, and the framework came in handy as I learned the restrictions of each new pci controller model. The behavior now is that if libvirt is auto-assigning a slot for a device, it will put it into a hotpluggable true PCI slot, but if you manually assign it to a slot that is non-hotpluggable and/or PCIe, it will be allowed. But when i tried to manually assign virtio-PCI to PCIe i simply got Device requires standard PCI slot and that was all. I had to make patch N4 in order to overcome this. I'm glad you pointed out this patch (I had skipped over it), because 1) that patch is completely unnecessary ever since commits 1e15be1 and 9a12b6c were pushed upstream, and 2) that patch will cause problems with auto-assignment of addresses for virtio-net devices on Q35 machines (they will be put on pcie-root instead of the pci-bridge). I have verified that (1) is true - I removed your patch, built and installed new libvirt, and tried adding a new virtio-net device to Cole's aarch64 example domain with a manually set pci address on both bus 0 (pcie-root) and bus 1 (dmi-to-pci-bridge), and both were successful. I just sent a patch that reverts your patch 4. Please test it to verify my claims and let me know so I can push it. https://www.redhat.com/archives/libvir-list/2015-August/msg00488.html BTW, I'm still wondering if the arm machinetype really does support the supposedly Interl-x86-specific i82801b11-bridge device Yes, it works fine. Just devices behind it cannot get MSI-X enabled. I'm not expert enough to know for sure, but that sounds like a bug. Alex? I do recall that a very long time ago
Re: [libvirt] [PATCH v5 0/4] qemu: Allow PCI virtio on ARM virt machine
Hello! The original patches to support pcie-root severely restricted what could plug into what because in real hardware you can't plug a PCI device into a PCIe slot (physically it doesn't work) But how do you know whether the device is PCI or PCIe ? I don't see anything like this in the code, i see that for example all network cards are PCI, which is, BTW, not true in the real world. The behavior now is that if libvirt is auto-assigning a slot for a device, it will put it into a hotpluggable true PCI slot, but if you manually assign it to a slot that is non-hotpluggable and/or PCIe, it will be allowed. But when i tried to manually assign virtio-PCI to PCIe i simply got Device requires standard PCI slot and that was all. I had to make patch N4 in order to overcome this. BTW, I'm still wondering if the arm machinetype really does support the supposedly Interl-x86-specific i82801b11-bridge device Yes, it works fine. Just devices behind it cannot get MSI-X enabled. By the way, you should be using virtio-pci with PC guests for a while, does it also suffer from this restriction there ? (and the new controller devices - ioh3420 (pcie-root-port), x3130-upstream (pcie-switch-upstream-port), and xio3130-downstream (pcie-switch-downstream-port). Didn't try that, but don't see why they would not work. PCI is just PCI after all, everything behing the controller is pretty much standard and arch-independent. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/6] admin API: Introduce server listing API
I'll send virt-admin list call once the virsh split series is merged.
Erik Skultety (6):
test: Replace tabs with spaces in input-data-admin-nomdns.json
rpc: Introduce new elements 'id' and 'name' to virnetserver structure
admin: Drop 'internal.h' include from libvirt-admin.h
admin: Move admin_server.{h,c} to admin.{h,c}
admin: Introduce adminDaemonConnectListServers API
admin: Usage example of the new server listing API
.gitignore | 1 +
Makefile.am| 2 +-
configure.ac | 1 +
daemon/Makefile.am | 4 +-
daemon/admin.c | 174 +++
daemon/admin.h | 36 +++
daemon/admin_server.c | 116 --
daemon/admin_server.h | 23 +-
daemon/libvirtd.c | 6 +-
examples/admin/Makefile.am | 25 +++
examples/admin/listservers.c | 73 ++
include/libvirt/libvirt-admin.h| 14 +-
po/POTFILES.in | 2 +-
src/admin/admin_protocol.x | 27 ++-
src/datatypes.c| 35 +++
src/datatypes.h| 35 +++
src/libvirt-admin.c| 173 +++
src/libvirt_admin.syms | 4 +
src/locking/lock_daemon.c | 2 +-
src/lxc/lxc_controller.c | 2 +-
src/rpc/virnetdaemon.c | 16 ++
src/rpc/virnetdaemon.h | 2 +
src/rpc/virnetserver.c | 41 +++-
src/rpc/virnetserver.h | 4 +
.../virnetdaemondata/input-data-admin-nomdns.json | 246 +++--
.../virnetdaemondata/input-data-anon-clients.json | 1 +
.../input-data-initial-nomdns.json | 1 +
tests/virnetdaemondata/input-data-initial.json | 1 +
tests/virnetdaemontest.c | 2 +-
29 files changed, 846 insertions(+), 223 deletions(-)
create mode 100644 daemon/admin.c
create mode 100644 daemon/admin.h
create mode 100644 examples/admin/Makefile.am
create mode 100644 examples/admin/listservers.c
--
2.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] rpc: Introduce new elements 'id' and 'name' to virnetserver structure
By adding these elements, we'll be able to represent the servers on
client side. This is merely because when listing clients or managing
clients, it would be convenient to know which server they're connected
to. Also reflect this change in virnetdaemontest as well.
---
daemon/libvirtd.c | 2 ++
src/locking/lock_daemon.c | 2 +-
src/lxc/lxc_controller.c | 2 +-
src/rpc/virnetdaemon.c| 1 +
src/rpc/virnetserver.c| 17 -
src/rpc/virnetserver.h| 1 +
tests/virnetdaemondata/input-data-admin-nomdns.json | 2 ++
tests/virnetdaemondata/input-data-anon-clients.json | 1 +
tests/virnetdaemondata/input-data-initial-nomdns.json | 1 +
tests/virnetdaemondata/input-data-initial.json| 1 +
tests/virnetdaemontest.c | 2 +-
11 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 250094b..88ad753 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1390,6 +1390,7 @@ int main(int argc, char **argv) {
config-keepalive_interval,
config-keepalive_count,
config-mdns_adv ? config-mdns_name : NULL,
+default,
remoteClientInitHook,
NULL,
remoteClientFreeFunc,
@@ -1464,6 +1465,7 @@ int main(int argc, char **argv) {
config-admin_keepalive_interval,
config-admin_keepalive_count,
NULL,
+ admin,
remoteAdmClientInitHook,
NULL,
remoteAdmClientFreeFunc,
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index c035024..6e67620 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -151,7 +151,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool
privileged)
if (!(lockd-srv = virNetServerNew(1, 1, 0, config-max_clients,
config-max_clients, -1, 0,
- NULL,
+ NULL, virlockd,
virLockDaemonClientNew,
virLockDaemonClientPreExecRestart,
virLockDaemonClientFree,
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 48a3597..0984be0 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -926,7 +926,7 @@ static int virLXCControllerSetupServer(virLXCControllerPtr
ctrl)
if (!(srv = virNetServerNew(0, 0, 0, 1,
0, -1, 0,
-NULL,
+NULL, LXC,
virLXCControllerClientPrivateNew,
NULL,
virLXCControllerClientPrivateFree,
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 910f266..bdfcfb7 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -69,6 +69,7 @@ struct _virNetDaemon {
int sigwrite;
int sigwatch;
+unsigned int nextSrvId;
size_t nservers;
virNetServerPtr *servers;
virJSONValuePtr srvObject;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 80b5588..ae48cbb 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -36,6 +36,7 @@
VIR_LOG_INIT(rpc.netserver);
+static unsigned int nextServerId;
typedef struct _virNetServerJob virNetServerJob;
typedef virNetServerJob *virNetServerJobPtr;
@@ -49,6 +50,8 @@ struct _virNetServerJob {
struct _virNetServer {
virObjectLockable parent;
+char *name;
+unsigned int id;
virThreadPoolPtr workers;
char *mdnsGroupName;
@@ -312,6 +315,7 @@ virNetServerPtr virNetServerNew(size_t min_workers,
int keepaliveInterval,
unsigned int keepaliveCount,
const char *mdnsGroupName,
+const char *serverName,
virNetServerClientPrivNew clientPrivNew,
virNetServerClientPrivPreExecRestart
clientPrivPreExecRestart,
virFreeCallback clientPrivFree,
@@ -332,6 +336,7 @@ virNetServerPtr virNetServerNew(size_t min_workers,
srv)))
goto error;
+srv-id = nextServerId++;
srv-nclients_max = max_clients;
srv-nclients_unauth_max =
Re: [libvirt] [PATCH v3 08/20] cpu: Never skip CPU model name check in ppc64 driver
On Mon, Aug 10, 2015 at 10:55:50 +0200, Andrea Bolognani wrote: ppc64Compute(), called by cpuNodeData(), is used not only to retrieve the driver-specific data associated to a guest CPU definition, but also to check whether said guest CPU is compatible with the host CPU. If the user is not interested in the CPU data, it's perfectly fine to pass a NULL pointer instead of a return location, and the compatibility data returned should not be affected by this. One of the checks, specifically the one on CPU model name, was however only performed if the return location was non-NULL. --- src/cpu/cpu_ppc64.c | 31 +++ 1 file changed, 15 insertions(+), 16 deletions(-) ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/20] cpu: Use ppc64Compute() to implement ppc64DriverCompare()
On Mon, Aug 10, 2015 at 10:55:52 +0200, Andrea Bolognani wrote: This ensures comparison of two CPU definitions will be consistent regardless of the fact that it is performed using cpuCompare() or cpuGuestData(). The x86 driver uses the same exact code. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert LXC: show used memory as 0 when domain is not active
On Mon, Aug 10, 2015 at 12:49:55PM -0600, Jim Fehlig wrote: This reverts commit 1ce7c1d20cfd5afb26d2dbc88201085d52415d0e, which introduced a significant semantic change to the virDomainGetInfo() API. Additionally, the change was only made to 2 of the 15 virt drivers. Conflicts: src/qemu/qemu_driver.c Signed-off-by: Jim Fehlig jfeh...@suse.com --- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 12 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) ACK I guess we should probably do this in the stable branch too, since I think it made it into the most recent release. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.
On 08/11/2015 11:14 AM, Simon Kelley wrote:
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
On 10/08/15 22:29, Laine Stump wrote:
On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote:
This is a fix for commit
db488c79173b240459c7754f38c3c6af9b432970 dnsmasq main process
exits without waiting for DAD, this is dnsmasq daemon's task. So
we periodically poll the kernel using netlink and check whether
there are any IPv6 addresses assigned to bridge which have
'tentative' state. After DAD is finished, execution continues. I
guess that is what dnsmasq was assumed to do.
Since the comments in our code imply that dnsmasq should be waiting
for DAD to complete prior to daemonizing, before pushing a fix like
this I'd like to find out from the dnsmasq folks if we are
erroneously relying on nonexistent dnsmasq behavior, or if maybe
there is a bug in some version of dnsmasq.
Simon (or other dnsmasq people) - when dnsmasq is run with
enable-ra, does it make sure it completes DAD prior to
daemonizing? Or does libvirt need to do this extra polling to
assure that DAD has completed? (or maybe there's some other config
parameter we need to add?)
Dnsmasq doesn't wait for DAD to complete before returning. Internally,
it know is DAD is still happening on an interface, as it needs to
delay calling bind() until after it's complete. It would, therefore be
relatively simple to add this behaviour, but it's not a complete
solution, since new interfaces can appear _after_ dnsmasq has
completed start-up.
Having libvirt do its own checks whenever it creates an interface
might therefore be a cleaner way of doing things, but I don't have an
objection to changing dnsmasq behaviour if there's a good reason why
that's not sensible.
I think this behavior will be inconsistent with the behavior present in
dnsmasq for long, so we can create conflicts with software relying on
current dnsmasq logic. I think if libvirt needs dad to finish then it
should wait itself instead of modifying third-party software.
Cheers,
Simon.
--- Difference to v2: Moved to virnetdev.
src/libvirt_private.syms| 1 + src/network/bridge_driver.c |
35 +- src/util/virnetdev.c| 160
src/util/virnetdev.h
| 2 + 4 files changed, 197 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0517c24..fa9e1c1 100644 --- a/src/libvirt_private.syms +++
b/src/libvirt_private.syms @@ -1776,6 +1776,7 @@
virNetDevSetRcvMulti; virNetDevSetupControl; virNetDevSysfsFile;
virNetDevValidateConfig; +virNetDevWaitDadFinish;
# util/virnetdevbandwidth.h diff --git
a/src/network/bridge_driver.c b/src/network/bridge_driver.c index
3d6721b..2172a3d 100644 --- a/src/network/bridge_driver.c +++
b/src/network/bridge_driver.c @@ -2026,6 +2026,33 @@
networkAddRouteToBridge(virNetworkObjPtr network, }
static int +networkWaitDadFinish(virNetworkObjPtr network) +{ +
virNetworkIpDefPtr ipdef; +virSocketAddrPtr *addrs = NULL; +
size_t i; +int ret; +for (i = 0; + (ipdef =
virNetworkDefGetIpByIndex(network-def, AF_INET6, i)); +
i++) {} + +if (i == 0) +return 0; +if
(VIR_ALLOC_N(addrs, i)) +return -1; + +for (i = 0; +
(ipdef = virNetworkDefGetIpByIndex(network-def, AF_INET6, i)); +
i++) { +addrs[i] = ipdef-address; +} + +ret =
virNetDevWaitDadFinish(addrs, i); +VIR_FREE(addrs); +
return ret; +} + +static int
networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
virNetworkObjPtr network) { @@ -2159,7 +2186,13 @@
networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if
(v6present networkStartRadvd(driver, network) 0) goto err4;
-/* DAD has happened (dnsmasq waits for it), dnsmasq is now
bound to the +/* dnsmasq main process does not wait for DAD
to complete, + * so we need to wait for it ourselves. +
*/ +if (v6present networkWaitDadFinish(network) 0) +
goto err4; + +/* DAD has happened, dnsmasq is now bound to
the * bridge's IPv6 address, so we can now set the dummy tun
down. */ if (tapfd = 0) { diff --git a/src/util/virnetdev.c
b/src/util/virnetdev.c index 1e20789..c81342a 100644 ---
a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -96,6 +96,7
@@ VIR_LOG_INIT(util.netdev); # define
FEATURE_BIT_IS_SET(blocks, index, field)\
(FEATURE_WORD(blocks, index, field) FEATURE_FIELD_FLAG(index))
#endif +# define IP_BUF_SIZE 4096
typedef enum { VIR_MCAST_TYPE_INDEX_TOKEN, @@ -1219,6 +1220,103
@@ int virNetDevClearIPAddress(const char *ifname, return ret; }
+/* return whether there is a known address with 'tentative' flag
set */ +static int +virNetDevParseDadStatus(struct nlmsghdr *nlh,
int len, +virSocketAddrPtr *addrs, size_t
count) +{ +struct ifaddrmsg *ifaddrmsg_ptr; +unsigned int
ifaddrmsg_len; +struct rtattr *rtattr_ptr; +size_t i; +
struct in6_addr *addr; +for (; NLMSG_OK(nlh, len); nlh =
NLMSG_NEXT(nlh, len)) { +if
[libvirt] [PATCH 1/6] test: Replace tabs with spaces in input-data-admin-nomdns.json
JSON data that are used to initialize tests in virnetdaemontest should
be in a consistent format, i.e. not using tabs for indentation, those
should be replaced by spaces.
---
.../virnetdaemondata/input-data-admin-nomdns.json | 244 ++---
1 file changed, 122 insertions(+), 122 deletions(-)
diff --git a/tests/virnetdaemondata/input-data-admin-nomdns.json
b/tests/virnetdaemondata/input-data-admin-nomdns.json
index 59bc471..449bcc9 100644
--- a/tests/virnetdaemondata/input-data-admin-nomdns.json
+++ b/tests/virnetdaemondata/input-data-admin-nomdns.json
@@ -1,126 +1,126 @@
{
servers: [
- {
- min_workers: 10,
- max_workers: 50,
- priority_workers: 5,
- max_clients: 100,
- keepaliveInterval: 120,
- keepaliveCount: 5,
- keepaliveRequired: true,
- services: [
- {
- auth: 0,
- readonly: true,
- nrequests_client_max: 2,
- socks: [
- {
- fd: 100,
- errfd: -1,
- pid: 0,
- isClient: false
- }
- ]
- },
- {
- auth: 2,
- readonly: false,
- nrequests_client_max: 5,
- socks: [
- {
- fd: 101,
- errfd: -1,
- pid: 0,
- isClient: false
- }
- ]
- }
- ],
- clients: [
- {
- auth: 1,
- readonly: true,
- nrequests_max: 15,
- sock: {
- fd: 102,
- errfd: -1,
- pid: -1,
- isClient: true
- }
- },
- {
- auth: 2,
- readonly: true,
- nrequests_max: 66,
- sock: {
- fd: 103,
- errfd: -1,
- pid: -1,
- isClient: true
- }
- }
- ]
- },
- {
- min_workers: 2,
- max_workers: 50,
- priority_workers: 5,
- max_clients: 100,
- keepaliveInterval: 120,
- keepaliveCount: 5,
- keepaliveRequired: true,
- services: [
- {
- auth: 0,
- readonly: true,
- nrequests_client_max: 2,
- socks: [
- {
- fd: 100,
- errfd: -1,
- pid: 0,
- isClient: false
- }
- ]
- },
- {
- auth: 2,
- readonly: false,
- nrequests_client_max: 5,
- socks: [
- {
- fd: 101,
- errfd: -1,
- pid: 0,
- isClient: false
- }
- ]
- }
- ],
- clients: [
- {
- auth: 1,
- readonly: true,
- nrequests_max: 15,
- sock: {
- fd: 102,
- errfd: -1,
- pid: -1,
- isClient: true
- }
- },
- {
- auth: 2,
- readonly: true,
- nrequests_max: 66,
- sock: {
- fd: 103,
- errfd: -1,
- pid: -1,
- isClient: true
- }
- }
- ]
- }
+{
+min_workers: 10,
+max_workers: 50,
+priority_workers: 5,
+max_clients: 100,
+keepaliveInterval: 120,
+keepaliveCount: 5,
+keepaliveRequired: true,
+services: [
+{
+auth: 0,
+readonly: true,
+nrequests_client_max: 2,
+socks: [
+{
+fd: 100,
+errfd: -1,
+pid: 0,
+isClient: false
+}
+]
+},
+{
+auth: 2,
+readonly: false,
+nrequests_client_max: 5,
+socks: [
+{
+fd: 101,
+errfd: -1,
+pid: 0,
+isClient: false
+}
+
[libvirt] [PATCH 5/6] admin: Introduce adminDaemonConnectListServers API
This is the first API to the admin interface. This particular API is
a convenience API, i.e. when managing clients connected to daemon's servers,
we should know (convenience) which server the specific client is connected to.
This implies a client-side representation of a server along with a basic
API to let the administrating client know what servers are actually
available on the daemon.
---
daemon/Makefile.am | 2 +-
daemon/admin.c | 57 +
daemon/admin_server.c | 83 +++
daemon/admin_server.h | 33
include/libvirt/libvirt-admin.h | 12 +++
src/admin/admin_protocol.x | 27 ++-
src/datatypes.c | 35
src/datatypes.h | 35
src/libvirt-admin.c | 173
src/libvirt_admin.syms | 4 +
src/rpc/virnetdaemon.c | 15
src/rpc/virnetdaemon.h | 2 +
src/rpc/virnetserver.c | 24 ++
src/rpc/virnetserver.h | 3 +
14 files changed, 503 insertions(+), 2 deletions(-)
create mode 100644 daemon/admin_server.c
create mode 100644 daemon/admin_server.h
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 5637f5a..d4dd4ac 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -128,7 +128,7 @@ libvirtd_conf_la_LIBADD = $(LIBXML_LIBS)
noinst_LTLIBRARIES += libvirtd_admin.la
libvirtd_admin_la_SOURCES = \
- admin.c admin.h
+ admin.c admin.h admin_server.c admin_server.h
libvirtd_admin_la_CFLAGS = \
$(AM_CFLAGS)\
diff --git a/daemon/admin.c b/daemon/admin.c
index ef404a9..d765231 100644
--- a/daemon/admin.c
+++ b/daemon/admin.c
@@ -28,6 +28,7 @@
#include admin_protocol.h
#include admin.h
+#include admin_server.h
#include datatypes.h
#include viralloc.h
#include virerror.h
@@ -77,6 +78,16 @@ remoteAdmClientInitHook(virNetServerClientPtr client
ATTRIBUTE_UNUSED,
return priv;
}
+/* Helpers */
+
+static void
+make_nonnull_server(admin_nonnull_server *srv_dst,
+virAdmServerPtr srv_src)
+{
+srv_dst-id = srv_src-id;
+ignore_value(VIR_STRDUP_QUIET(srv_dst-name, srv_src-name));
+}
+
/* Functions */
static int
adminDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
@@ -114,4 +125,50 @@ adminDispatchConnectClose(virNetServerPtr server
ATTRIBUTE_UNUSED,
return 0;
}
+
+static int
+adminDispatchConnectListServers(virNetServerPtr server ATTRIBUTE_UNUSED,
+virNetServerClientPtr client,
+virNetMessagePtr msg ATTRIBUTE_UNUSED,
+virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+admin_connect_list_servers_args *args,
+admin_connect_list_servers_ret *ret)
+{
+virAdmServerPtr *servers = NULL;
+int nservers = 0;
+int rv = -1;
+size_t i;
+struct daemonAdmClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+
+if ((nservers =
+adminDaemonConnectListServers(priv-dmn,
+ args-need_results ? servers : NULL,
+ args-flags)) 0)
+goto cleanup;
+
+if (servers nservers) {
+if (VIR_ALLOC_N(ret-servers.servers_val, nservers) 0)
+goto cleanup;
+
+ret-servers.servers_len = nservers;
+for (i = 0; i nservers; i++)
+make_nonnull_server(ret-servers.servers_val + i, servers[i]);
+} else {
+ret-servers.servers_len = 0;
+ret-servers.servers_val = NULL;
+}
+
+ret-ret = nservers;
+rv = 0;
+
+ cleanup:
+if (rv 0)
+virNetMessageSaveError(rerr);
+if (servers nservers 0)
+for (i = 0; i nservers; i++)
+virObjectUnref(servers[i]);
+VIR_FREE(servers);
+return rv;
+}
#include admin_dispatch.h
diff --git a/daemon/admin_server.c b/daemon/admin_server.c
new file mode 100644
index 000..9af94ee
--- /dev/null
+++ b/daemon/admin_server.c
@@ -0,0 +1,83 @@
+/*
+ * admin_server.c: admin methods to manage daemons and clients
+ *
+ * Copyright (C) 2014-2015 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
+ * http://www.gnu.org/licenses/.
+ *
+ *
[libvirt] [PATCH 4/6] admin: Move admin_server.{h, c} to admin.{h, c}
This change is merely because admin_server would contain all the code
from dispatchers and helpers to the actual APIs. Admin should have
similar structure to the daemon-side remote driver - dispatchers and
helpers in a separate module, APIs in a separate module.
---
daemon/Makefile.am| 4 +-
daemon/admin.c| 117 ++
daemon/admin.h| 36
daemon/admin_server.c | 117 --
daemon/admin_server.h | 36
daemon/libvirtd.c | 4 +-
po/POTFILES.in| 2 +-
7 files changed, 158 insertions(+), 158 deletions(-)
create mode 100644 daemon/admin.c
create mode 100644 daemon/admin.h
delete mode 100644 daemon/admin_server.c
delete mode 100644 daemon/admin_server.h
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 59bc4d4..5637f5a 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -128,7 +128,7 @@ libvirtd_conf_la_LIBADD = $(LIBXML_LIBS)
noinst_LTLIBRARIES += libvirtd_admin.la
libvirtd_admin_la_SOURCES = \
- admin_server.c admin_server.h
+ admin.c admin.h
libvirtd_admin_la_CFLAGS = \
$(AM_CFLAGS)\
@@ -319,7 +319,7 @@ endif ! WITH_POLKIT
remote.c: $(DAEMON_GENERATED)
remote.h: $(DAEMON_GENERATED)
-admin_server.c: $(DAEMON_GENERATED)
+admin.c: $(DAEMON_GENERATED)
LOGROTATE_CONFS = libvirtd.qemu.logrotate libvirtd.lxc.logrotate \
libvirtd.libxl.logrotate libvirtd.uml.logrotate \
diff --git a/daemon/admin.c b/daemon/admin.c
new file mode 100644
index 000..ef404a9
--- /dev/null
+++ b/daemon/admin.c
@@ -0,0 +1,117 @@
+/*
+ * admin.c: handlers for RPC method calls
+ *
+ * Copyright (C) 2014-2015 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
+ * http://www.gnu.org/licenses/.
+ *
+ * Author: Martin Kletzander mklet...@redhat.com
+ */
+
+#include config.h
+
+#include internal.h
+#include libvirtd.h
+#include libvirt_internal.h
+
+#include admin_protocol.h
+#include admin.h
+#include datatypes.h
+#include viralloc.h
+#include virerror.h
+#include virlog.h
+#include virnetdaemon.h
+#include virnetserver.h
+#include virstring.h
+#include virthreadjob.h
+
+#define VIR_FROM_THIS VIR_FROM_ADMIN
+
+VIR_LOG_INIT(daemon.admin);
+
+
+void
+remoteAdmClientFreeFunc(void *data)
+{
+struct daemonAdmClientPrivate *priv = data;
+
+virMutexDestroy(priv-lock);
+virObjectUnref(priv-dmn);
+VIR_FREE(priv);
+}
+
+void *
+remoteAdmClientInitHook(virNetServerClientPtr client ATTRIBUTE_UNUSED,
+void *opaque)
+{
+struct daemonAdmClientPrivate *priv;
+
+if (VIR_ALLOC(priv) 0)
+return NULL;
+
+if (virMutexInit(priv-lock) 0) {
+VIR_FREE(priv);
+virReportSystemError(errno, %s, _(unable to init mutex));
+return NULL;
+}
+
+/*
+ * We don't necessarily need to ref this object right now as there
+ * must be one ref being held throughout the life of the daemon,
+ * but let's just be safe for future.
+ */
+priv-dmn = virObjectRef(opaque);
+
+return priv;
+}
+
+/* Functions */
+static int
+adminDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ struct admin_connect_open_args *args)
+{
+unsigned int flags;
+struct daemonAdmClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+int ret = -1;
+
+VIR_DEBUG(priv=%p dmn=%p, priv, priv-dmn);
+virMutexLock(priv-lock);
+
+flags = args-flags;
+virCheckFlagsGoto(0, cleanup);
+
+ret = 0;
+ cleanup:
+if (ret 0)
+virNetMessageSaveError(rerr);
+virMutexUnlock(priv-lock);
+return ret;
+}
+
+static int
+adminDispatchConnectClose(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED)
+{
+virNetServerClientDelayedClose(client);
+return 0;
+}
+
+#include admin_dispatch.h
diff --git a/daemon/admin.h b/daemon/admin.h
new file mode 100644
index 000..6262026
--- /dev/null
+++
[libvirt] [PATCH 6/6] admin: Usage example of the new server listing API
Not to be actually pushed since majority of this example will be merged
into virt-admin once it's ready, i.e. virsh splitting series is merged,
but might be good to just see the API's working.
---
.gitignore | 1 +
Makefile.am | 2 +-
configure.ac | 1 +
examples/admin/Makefile.am | 25 +++
examples/admin/listservers.c | 73
5 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 examples/admin/Makefile.am
create mode 100644 examples/admin/listservers.c
diff --git a/.gitignore b/.gitignore
index 0b40f4a..325f04f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
/docs/libvirt-refs.xml
/docs/search.php
/docs/todo.html.in
+/examples/admin/listservers
/examples/object-events/event-test
/examples/dominfo/info1
/examples/domsuspend/suspend
diff --git a/Makefile.am b/Makefile.am
index 91b943b..c14229e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -24,7 +24,7 @@ SUBDIRS = . gnulib/lib include src daemon tools docs
gnulib/tests \
examples/dominfo examples/domsuspend examples/apparmor \
examples/xml/nwfilter examples/openauth examples/systemtap \
tools/wireshark examples/dommigrate \
- examples/lxcconvert examples/domtop
+ examples/lxcconvert examples/domtop examples/admin
ACLOCAL_AMFLAGS = -I m4
diff --git a/configure.ac b/configure.ac
index 46c80ce..35c8cd9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2805,6 +2805,7 @@ AC_CONFIG_FILES([\
examples/systemtap/Makefile \
examples/xml/nwfilter/Makefile \
examples/lxcconvert/Makefile \
+examples/admin/Makefile \
tools/wireshark/Makefile \
tools/wireshark/src/Makefile])
AC_OUTPUT
diff --git a/examples/admin/Makefile.am b/examples/admin/Makefile.am
new file mode 100644
index 000..8373132
--- /dev/null
+++ b/examples/admin/Makefile.am
@@ -0,0 +1,25 @@
+## Copyright (C) 2005-2015 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
+## http://www.gnu.org/licenses/.
+
+INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include
+LDADDS = $(STATIC_BINARIES) $(WARN_CFLAGS) \
+$(top_builddir)/src/libvirt-admin.la $(COVERAGE_LDFLAGS)
+
+noinst_PROGRAMS=listservers
+
+listservers_SOURCES=listservers.c
+listservers_LDFLAGS=
+listservers_LDADD= $(LDADDS)
diff --git a/examples/admin/listservers.c b/examples/admin/listservers.c
new file mode 100644
index 000..6f11d8d
--- /dev/null
+++ b/examples/admin/listservers.c
@@ -0,0 +1,73 @@
+/*
+ * listservers.c: Demo program to show listing of available servers
+ *
+ * Copyright (C) 2015 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
+ * http://www.gnu.org/licenses/.
+ *
+ * Author: Erik Skultety eskul...@redhat.com
+ */
+
+#include stdio.h
+#include stdlib.h
+#include libvirt/libvirt-admin.h
+
+static int
+listDaemonServers(void)
+{
+int ret = -1;
+virAdmConnectPtr conn = NULL;
+virAdmServerPtr *srvs = NULL;
+int nsrvs = 0;
+size_t i;
+
+/* Connect to an admin server on a specific daemon, NULL in this case means
+ * connect to libvirtd UNIX socket
+ */
+if (!(conn = virAdmConnectOpen(NULL, 0))) {
+fprintf(stderr, Failed to connect to the admin server\n);
+goto cleanup;
+}
+
+/* Obtain a list of available servers on the daemon */
+if ((nsrvs = virAdmConnectListServers(conn, srvs, 0)) 0) {
+fprintf(stderr, Failed to obtain list of available servers\n);
+goto cleanup;
+}
+
+printf( %-5s %-15s\n, Id, Name);
+printf(---\n);
+for (i = 0; i nsrvs; i++)
+printf( %-5d %-15s\n, virAdmGetServerID(srvs[i]),
+ virAdmGetServerName(srvs[i]));
+
+ret = nsrvs;
+
+ cleanup:
[libvirt] [PATCH 3/6] admin: Drop 'internal.h' include from libvirt-admin.h
This is a public library, it shouldn't include anything that is
internal. Including the library in it's current state to an example
application fails the preprocessor phase.
---
include/libvirt/libvirt-admin.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index b3cfc93..9997cc2 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -26,8 +26,6 @@
#ifndef __VIR_ADMIN_H__
# define __VIR_ADMIN_H__
-# include internal.h
-
# ifdef __cplusplus
extern C {
# endif
--
2.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
Hi! The only negative impact it would have is that if someone has old QEMU installed at define time and later upgrades to a QEMU with PCI support before starting the guest, they'll lack the PCI controller. This won't actually happen, because together with the missing controller he/she will not have PCI devices. Attempt to add a device without a bus immediately produces No PCI bus report, and the XML gets rejected. And, right after the upgrade, as soon as he/she adds some PCI devices to use, the XML will be re-parsed, and PCI bus description will be added. Because reparsing happens after any change. So from that POV, maybe we should just clean up the current merged patch so that it is properly exercised in the test suite by providing capabilities I will try to do it soon. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] qemu: Implement VIR_DOMAIN_BANDWIDTH_IN_FLOOR
On 11.08.2015 03:31, John Ferlan wrote:
On 08/03/2015 02:39 AM, Michal Privoznik wrote:
Well, there are just two places that needs adjustment:
qemuDomainGetInterfaceParameters - to report the @floor
qemuDomainSetInterfaceParameters - now that the function has been
fixed, we can allow updating @floor too.
Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
src/qemu/qemu_driver.c | 23 ++-
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 171b58f..7123083 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -138,7 +138,7 @@ VIR_LOG_INIT(qemu.qemu_driver);
#define QEMU_NB_BLKIO_PARAM 6
-#define QEMU_NB_BANDWIDTH_PARAM 6
+#define QEMU_NB_BANDWIDTH_PARAM 7
static void processWatchdogEvent(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -11126,6 +11126,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
VIR_TYPED_PARAM_UINT,
VIR_DOMAIN_BANDWIDTH_IN_BURST,
VIR_TYPED_PARAM_UINT,
+ VIR_DOMAIN_BANDWIDTH_IN_FLOOR,
+ VIR_TYPED_PARAM_UINT,
VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE,
VIR_TYPED_PARAM_UINT,
VIR_DOMAIN_BANDWIDTH_OUT_PEAK,
@@ -11178,6 +11180,9 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
bandwidth-in-peak = params[i].value.ui;
} else if (STREQ(param-field, VIR_DOMAIN_BANDWIDTH_IN_BURST)) {
bandwidth-in-burst = params[i].value.ui;
+} else if (STREQ(param-field, VIR_DOMAIN_BANDWIDTH_IN_FLOOR)) {
+bandwidth-in-floor = params[i].value.ui;
+inboundSpecified = true;
If 'average' is required, then can one really provide just floor?
} else if (STREQ(param-field, VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE)) {
bandwidth-out-average = params[i].value.ui;
outboundSpecified = true;
@@ -11191,7 +11196,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
/* average is mandatory, peak and burst are optional. So if no
* average is given, we free inbound/outbound here which causes
* inbound/outbound to not be set. */
Adjust comment to account for floor...
-if (!bandwidth-in-average)
+if (!bandwidth-in-average !bandwidth-in-floor)
Again, I thought average was required ... sorry it's just late. Maybe
the updated comment will answer my query...
Either average or floor is required. This is what I am squashing in:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d92c6e0..2e44500 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11197,9 +11197,9 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
}
}
-/* average is mandatory, peak and burst are optional. So if no
- * average is given, we free inbound/outbound here which causes
- * inbound/outbound to not be set. */
+/* average or floor are mandatory, peak and burst are optional.
+ * So if no average or floor is given, we free inbound/outbound
+ * here which causes inbound/outbound to not be set. */
if (!bandwidth-in-average !bandwidth-in-floor)
VIR_FREE(bandwidth-in);
if (!bandwidth-out-average)
ACK in principle
John
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] virsh: Implement VIR_DOMAIN_BANDWIDTH_IN_FLOOR
On 11.08.2015 03:25, John Ferlan wrote:
On 08/03/2015 02:39 AM, Michal Privoznik wrote:
We have a function parseRateStr() that parses --inbound and
--outbound arguments to both attach-interface and domiftune.
Now that we have all virTypedParams macros needed for QoS,
lets parse even floor attribute. The extended format for the
arguments looks like this then:
--inbound average[,peak[,burst[,floor]]]
Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
tools/virsh-domain.c | 23 ++-
tools/virsh.pod | 12 ++--
2 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index bb40ddd..5b7e623 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
Some comments above here need adjustment to describe the new field and
possible options...
Does it matter if someone provides floor on outbound (it's a testing
question ;-))
@@ -873,7 +873,7 @@ static int parseRateStr(vshControl *ctl,
char *next;
char *saveptr;
enum {
-AVERAGE, PEAK, BURST
+AVERAGE, PEAK, BURST, FLOOR
} state;
int ret = -1;
@@ -882,7 +882,7 @@ static int parseRateStr(vshControl *ctl,
next = vshStrdup(ctl, rateStr);
-for (state = AVERAGE; state = BURST; state++) {
+for (state = AVERAGE; state = FLOOR; state++) {
unsigned long long *tmp;
const char *field_name;
@@ -905,6 +905,11 @@ static int parseRateStr(vshControl *ctl,
tmp = rate-burst;
field_name = burst;
break;
+
+case FLOOR:
+tmp = rate-floor;
+field_name = floor;
+break;
}
if (virStrToLong_ullp(token, NULL, 10, tmp) 0) {
@@ -976,7 +981,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd)
memset(inbound, 0, sizeof(inbound));
if (parseRateStr(ctl, inboundStr, inbound) 0)
goto cleanup;
-if (inbound.average == 0) {
+if (!inbound.average !inbound.floor) {
vshError(ctl, _(inbound average is mandatory));
Why checking !inbound.floor?
What if someone does ,,,floor value
Yeah, I should have updated the error message too. At least one of average and
floor is required. Your example is perfectly valid and in fact I've just just
that during my testing when writing the patches.
goto cleanup;
}
Also should we check below here for outboundStr having floor? (improperly)
We can, but so far the outbound.floor is just ignored.
@@ -3308,8 +3313,10 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
UINT_MAX);
goto cleanup;
}
-if (inbound.average == 0 (inbound.burst || inbound.peak)) {
-vshError(ctl, _(inbound average is mandatory));
+
+if ((!inbound.average (inbound.burst || inbound.peak))
+!inbound.floor) {
+vshError(ctl, _(either inbound average or floor is
mandatory));
goto cleanup;
}
@@ -3329,6 +3336,12 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
VIR_DOMAIN_BANDWIDTH_IN_BURST,
inbound.burst) 0)
goto save_error;
+
+if (inbound.floor
+virTypedParamsAddUInt(params, nparams, maxparams,
+ VIR_DOMAIN_BANDWIDTH_IN_FLOOR,
+ inbound.floor) 0)
+goto save_error;
}
if (outboundStr) {
...
should we check here if someone provides outbound.floor value and fail?
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 5ee9a96..a6148d3 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -770,7 +770,7 @@ Iinterface-device can be the interface's target name
or the MAC address.
=item Bdomiftune Idomain Iinterface-device
[[I--config] [I--live] | [I--current]]
-[I--inbound average,peak,burst]
+[I--inbound average,peak,burst,floor]
[I--outbound average,peak,burst]
Set or query the domain's network interface's bandwidth parameters.
@@ -779,9 +779,9 @@ or the MAC address.
If no I--inbound or I--outbound is specified, this command will
query and show the bandwidth settings. Otherwise, it will set the
-inbound or outbound bandwidth. Iaverage,peak,burst is the same as
-in command Iattach-interface. Values for Iaverage and Ipeak are
-expressed in kilobytes per second, while Iburst is expressed in kilobytes
+inbound or outbound bandwidth. Iaverage,peak,burst,floor is the same as
+in command Iattach-interface. Values for Iaverage, Ipeak and Ifloor
+are expressed in kilobytes per second, while Iburst is expressed in
kilobytes
in a single burst at -Ipeak speed as described in the Network XML
documentation at Lhttp://libvirt.org/formatnetwork.html#elementQoS.
@@ -2499,7 +2499,7 @@ Likewise,
Re: [libvirt] [PATCH 5/9] qemuDomainSetInterfaceParameters: Use new functions to update bandwidth
On 11.08.2015 03:06, John Ferlan wrote:
On 08/03/2015 02:39 AM, Michal Privoznik wrote:
As sketched in previous commits, imagine the following scenario:
virsh # domiftune gentoo vnet0
inbound.average: 100
inbound.peak : 0
inbound.burst : 0
outbound.average: 100
outbound.peak : 0
outbound.burst : 0
virsh # domiftune gentoo vnet0 --inbound 0
virsh # shutdown gentoo
Domain gentoo is being shutdown
virsh # list --all
error: Failed to list domains
error: Cannot recv data: Connection reset by peer
Program received signal SIGSEGV, Segmentation fault.
0x7fffe80ea221 in networkUnplugBandwidth (net=0x7fff9400c1a0,
iface=0x7fff940ea3e0) at network/bridge_driver.c:4881
4881net-floor_sum -= ifaceBand-in-floor;
This is rather unfortunate. We should not SIGSEGV here. The
problem is, that while in the second step the inbound QoS was
cleared out, the network part of it was not updated (moreover, we
don't report that vnet0 had inbound.floor set). Internal
structure therefore still had some fragments left (e.g.
class_id). So when qemuProcessStop() started to clean up the
environment it got to networkUnplugBandwidth(). Here, class_id is
set therefore function assumes that there is an inbound QoS. This
actually is a fair assumption to make, there's no need for a
special QoS box in network's QoS when there's no QoS to set.
Anyway, the problem is not the networkUnplugBandwidth() rather
than qemuDomainSetInterfaceParameters() which completely forgot
about QoS being disperse (some parts are set directly on
interface itself, some on bridge the interface is plugged into).
Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
src/qemu/qemu_driver.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b9278f8..171b58f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -100,6 +100,7 @@
#include vircgroup.h
#include virnuma.h
#include dirname.h
+#include network/bridge_driver.h
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -11221,7 +11222,11 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth-out));
}
-if (virNetDevBandwidthSet(net-ifname, newBandwidth, false) 0) {
+if (!networkBandwidthChangeAllowed(net, newBandwidth))
+goto endjob;
+
+if (virNetDevBandwidthSet(net-ifname, newBandwidth, false) 0 ||
+networkBandwidthUpdate(net, newBandwidth) 0) {
Is this the only caller of virNetDevBandwidthSet that needs the
ChangeAllowed check first? I think the answer is no based on your
explanation in the commit message, but figured I'd ask... Tough to keep
track of the multiple callers and what they're checking before calling
the Set and/or if they need to Update as well (it's been a long day ;-),
but I figured I'd at least look...
Yes and no. You are right that we should check if new bandwidth can be
fulfilled prior to setting it. And now, with this patch pushed we are
doing that. I admit that it's hard to see since the checks are on a
different places - for instance, in networkAllocateActualDevice() which
calls networkPlugBandwidth() eventually and that one finally calls
networkCheckBandwidth(). Maybe one day I'll pull up my sleeves and
refactor the code (to actually follow the advice I am going to give in
my talk on KVM Forum).
ACK for the change in any case.
John
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
Hello! I'd be inclined to go with option 2, and then if any PCI devices are actually used with the guest, check the capability at start time when we are doing auto-address assignment. I am following the discussion, sorry for being a bit silent... Just didn't have much to say. But what is actually wrong with using capabilities in post-parse? Anyway, devices do not have PCI addresses by default. They have them only if the user explicitly says so, to stay backwards-compatible with old guests. So, in a practical scenario the user would first upgrade qemu, then start adding PCI devices. And, while adding them, the user will get PCI controller in the description automatically. The only hypothetical scenario i could imagine is downgrading qemu. But anyway, in this case the user would have to manually remove PCI devices from the domain XML. Can anybody suggest a scenario where adding PCI in post-parse actually harms? The only argument i heard was: we should not be using the QEMU capabilities when defining the XML. But why? Another option: add versioned 'virt' machine types to the next qemu release (like virt-2.5 etc.), and key libvirt enabling pci off of that. 1. virt machine (unversioned one) already has PCI. 2. qemu people don't want to have versioned virt. I already proposed this for GICv3 and they strictly refused. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Buffer overflow error when starting libvirtd
On 08/11/2015 02:13 AM, Laine Stump wrote: On 08/11/2015 02:06 AM, Laine Stump wrote: On 08/11/2015 01:30 AM, Ján Tomko wrote: On Mon, Aug 10, 2015 at 11:21:47PM -0400, Willard Dennis wrote: Compiled libvirt 1.2.18 on Ubuntu 12.04.5 (installed over 12.04 libvirt-bin 0.9.9 package files); when I try to start libvirtd, I get the following error / backtrace: $ sudo /usr/sbin/libvirtd *** stack smashing detected ***: /usr/sbin/libvirtd terminated === Backtrace: = /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f02fe6d3e57] /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x0)[0x7f02fe6d3e20] /usr/lib/libvirt.so.0(+0xaa359)[0x7f02fec4f359] /usr/lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0x99fe)[0x7f02f1fc59fe] /usr/lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xa672)[0x7f02f1fc6672] /usr/lib/libvirt.so.0(virStateInitialize+0xaf)[0x7f02fed1cd2f] /usr/sbin/libvirtd(+0x17d80)[0x7f02ff98dd80] /usr/lib/libvirt.so.0(+0xd25d2)[0x7f02fec775d2] /lib/x86_64-linux-gnu/libpthread.so.0(+0x7e9a)[0x7f02fe98fe9a] /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7f02fe6bd38d] Not sure if this may be a bug with 1.2.18, or a fault with my configure/compile/install process; please advise... This has been reported last week: https://www.redhat.com/archives/libvir-list/2015-August/msg00163.html And here is the patch proposed to fix it, waiting on review: https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html Willard - If you're able to build libvirt and test that patch, it would be greatly appreciated. Ah, wait. Actually I'm fairly certain that the patch that Jan refers to will *not* fix the problem. The patch that I'd like to see tested is the one that I posted just a few hours ago: https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html Or even better, the V2 of that patch which I just posted as a reply to the original: https://www.redhat.com/archives/libvir-list/2015-August/msg00392.html -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] admin: Move admin_server.{h, c} to admin.{h, c}
On Tue, Aug 11, 2015 at 09:58:59AM +0200, Erik Skultety wrote: This change is merely because admin_server would contain all the code from dispatchers and helpers to the actual APIs. Admin should have similar structure to the daemon-side remote driver - dispatchers and helpers in a separate module, APIs in a separate module. --- daemon/Makefile.am| 4 +- daemon/admin.c| 117 ++ daemon/admin.h| 36 daemon/admin_server.c | 117 -- daemon/admin_server.h | 36 daemon/libvirtd.c | 4 +- po/POTFILES.in| 2 +- 7 files changed, 158 insertions(+), 158 deletions(-) create mode 100644 daemon/admin.c create mode 100644 daemon/admin.h delete mode 100644 daemon/admin_server.c delete mode 100644 daemon/admin_server.h Either rename these with 'git mv', format the patch with -M or at least note in the commit message that this is best viewed with -M. Or all of those together :) signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/6] admin: Drop 'internal.h' include from libvirt-admin.h
On Tue, Aug 11, 2015 at 09:58:58AM +0200, Erik Skultety wrote:
This is a public library, it shouldn't include anything that is
internal. Including the library in it's current state to an example
application fails the preprocessor phase.
---
include/libvirt/libvirt-admin.h | 2 --
1 file changed, 2 deletions(-)
ACK as-is (independent of other patches, feel free to push this ASAP.
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index b3cfc93..9997cc2 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -26,8 +26,6 @@
#ifndef __VIR_ADMIN_H__
# define __VIR_ADMIN_H__
-# include internal.h
-
# ifdef __cplusplus
extern C {
# endif
--
2.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 15/20] cpu: Parse and use PVR masks in the ppc64 driver
On Mon, Aug 10, 2015 at 10:55:57 +0200, Andrea Bolognani wrote: Instead of relying on a hard-coded mask value, read it from the CPU map XML and use it when looking up models by PVR. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/20] cpu: CPU model names have to match on ppc64
On Mon, Aug 10, 2015 at 10:55:51 +0200, Andrea Bolognani wrote: Limitations of the POWER architecture mean that you can't run eg. a POWER7 guest on a POWER8 host when using KVM. This applies to all guests, not just those using VIR_CPU_MATCH_STRICT in the CPU definition; in fact, exact and strict CPU matching are basically the same on ppc64. This means, of course, that hosts using different CPUs have to be considered incompatible as well. Change ppc64Compute(), called by cpuGuestData(), to reflect this fact and update test cases accordingly. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1250977 ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
On Mon, Aug 10, 2015 at 12:08:02PM -0400, Laine Stump wrote:
On 08/10/2015 11:09 AM, Daniel P. Berrange wrote:
On Thu, Aug 06, 2015 at 07:46:58PM +0200, Martin Kletzander wrote:
On Thu, Aug 06, 2015 at 06:37:41PM +0200, Martin Kletzander wrote:
On Fri, Jul 17, 2015 at 02:27:45PM +0300, Pavel Fedin wrote:
Here we assume that if qemu supports generic PCI host controller,
it is a part of virt machine and can be used for adding PCI devices.
In qemu this is actually a PCIe bus, so we also declare multibus
capability so that 0'th bus is specified to qemu correctly as 'pcie.0'
Signed-off-by: Pavel Fedin p.fe...@samsung.com
---
src/qemu/qemu_capabilities.c | 8
src/qemu/qemu_domain.c | 17 +
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d570fdd..f3486c7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2138,6 +2138,14 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr
qemuCaps,
return false;
}
+if (ARCH_IS_ARM(def-os.arch)) {
+/* If 'virt' supports PCI, it supports multibus.
+ * No extra conditions here for simplicity.
+ */
So every ARM qemu with the virt machine type supports both PCI and
multiqueue? How about those virt-* for which you check below. That
might not be related, I'm just curious.
+if (STREQ(def-os.machine, virt))
+return true;
+}
+
return false;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b050a0..c7d14e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -981,7 +981,7 @@ virDomainXMLNamespace
virQEMUDriverDomainXMLNamespace = {
static int
qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
bool addDefaultUSB = true;
bool addImplicitSATA = false;
@@ -1030,12 +1030,21 @@ qemuDomainDefPostParse(virDomainDefPtr def,
break;
case VIR_ARCH_ARMV7L:
- addDefaultUSB = false;
- addDefaultMemballoon = false;
- break;
case VIR_ARCH_AARCH64:
addDefaultUSB = false;
addDefaultMemballoon = false;
+ if (STREQ(def-os.machine, virt) ||
+ STRPREFIX(def-os.machine, virt-)) {
+ virQEMUDriverPtr driver = opaque;
+
+ /* This condition is actually a (temporary) hack for test
suite which
+* does not create capabilities cache */
Few questions here. a) how temporary is this since you're not
removing it in this series? b) for what tests you need this hack and
what part of the below is the hack?
Moreover, you cannot use capabilities when defining an XML. The
emulator can change between the domain is defined and started, so you
cannot know with what emulator this will be started.
I see Michal (Cc'd) just pushed this, I probably just missed the mail
Of course I forgot, Cc'ing now.
I agree with your core statement that we should not be using the QEMU
capabilities when defining the XML. With all existing scenarios we have
been able to determine whether to add the implicit PCI controller based
on the machine type name only, because with every other QEMU arch when
doing such a major change as adding a PCI bus, they have created a new
machine type. The problem is that arm 'virt' machine type is not stable,
it is being changed arbitrarily in new QEMU releases :-(
So AFAIK, that leaves us with 3 choices
- Never add PCI controller at time the XML is defined, on the basis
that we have to be conservative in what we add to cope with old QEMU
- Always add PCI controller at time the XML is defined, on the basis
that most people will have new enough QEMU because ARM 'virt' machine
type is very much still in development, so no one will serously stick
with the older QEMU versions which lack PCI.
- Use the capabilities in XML post-parse to conditionally add the
PCI controller. This is what was currently merged
I don't think option 1 makes much sense as it'll harm ARM arch forever
more, to cope with QEMU versions that will almost never be used in
practice.
I'd be inclined to go with option 2, and then if any PCI devices are
actually used with the guest, check the capability at start time when
we are doing auto-address assignment.
Except that auto-address assignment happens after the parse, but before
we write the config to disk (i.e. we don't wait until the domain is
started. I'm guessing that was originally done because we didn't want to
be potentially re-writing the config to disk every time the domain was
started.) Also, we can't assume that no PCI devices used at start
means no PCI devices will ever be desired because hotplug.
Ok, so it
Re: [libvirt] [PATCHv3 2/2] Add support for multi-part netlink messages.
On 08/11/2015 12:15 AM, Laine Stump wrote: On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote: Such messages do not have NLMSG_ERROR or NLMSG_DONE type but they are valid responses. We test 'multi-partness' by looking for NLM_F_MULTI flag. --- src/util/virnetlink.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0052ef9..f02bb59 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -386,7 +386,9 @@ virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen) break; default: -goto malformed_resp; +/* We allow multipart messages. */ +if (!(resp-nlmsg_flags NLM_F_MULTI)) +goto malformed_resp; 1) It's interesting that they don't seem to define what type the message will be in these cases (it's not NLMSG_DONE or NLMSG_ERROR, and the only other standard types are NLMSG_NOOP and NLMSG_OVERRUN.) So what *is* the type in the case of a multipart message. I tried it myself (centos7) and the type was 20 (I don't remember, hex or decimal) which is unspecified so we should not rely on particular type. 2) Doesn't the presence of the NLM_F_MULTI flag indicate that we need to look for another packet, rather than just returning? It's been a long time since I looked at the details of the netlink message handling, but won't this constipate the socket? At the time we get there all parts of multipart message are received. This is handled internally (http://www.infradead.org/~tgr/libnl/doc/core.html). We might parse all parts but I think this is cumbersome and redundant work since it is unlikely for message to be broken in submessages. -- Your sincerely, Maxim Perevedentsev -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Buffer overflow error when starting libvirtd
On 08/11/2015 02:06 AM, Laine Stump wrote: On 08/11/2015 01:30 AM, Ján Tomko wrote: On Mon, Aug 10, 2015 at 11:21:47PM -0400, Willard Dennis wrote: Compiled libvirt 1.2.18 on Ubuntu 12.04.5 (installed over 12.04 libvirt-bin 0.9.9 package files); when I try to start libvirtd, I get the following error / backtrace: $ sudo /usr/sbin/libvirtd *** stack smashing detected ***: /usr/sbin/libvirtd terminated === Backtrace: = /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f02fe6d3e57] /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x0)[0x7f02fe6d3e20] /usr/lib/libvirt.so.0(+0xaa359)[0x7f02fec4f359] /usr/lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0x99fe)[0x7f02f1fc59fe] /usr/lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xa672)[0x7f02f1fc6672] /usr/lib/libvirt.so.0(virStateInitialize+0xaf)[0x7f02fed1cd2f] /usr/sbin/libvirtd(+0x17d80)[0x7f02ff98dd80] /usr/lib/libvirt.so.0(+0xd25d2)[0x7f02fec775d2] /lib/x86_64-linux-gnu/libpthread.so.0(+0x7e9a)[0x7f02fe98fe9a] /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7f02fe6bd38d] Not sure if this may be a bug with 1.2.18, or a fault with my configure/compile/install process; please advise... This has been reported last week: https://www.redhat.com/archives/libvir-list/2015-August/msg00163.html And here is the patch proposed to fix it, waiting on review: https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html Willard - If you're able to build libvirt and test that patch, it would be greatly appreciated. Ah, wait. Actually I'm fairly certain that the patch that Jan refers to will *not* fix the problem. The patch that I'd like to see tested is the one that I posted just a few hours ago: https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
On 08/11/2015 01:23 AM, Martin Kletzander wrote:
On Mon, Aug 10, 2015 at 07:23:07PM -0400, Cole Robinson wrote:
On 08/10/2015 11:09 AM, Daniel P. Berrange wrote:
On Thu, Aug 06, 2015 at 07:46:58PM +0200, Martin Kletzander wrote:
On Thu, Aug 06, 2015 at 06:37:41PM +0200, Martin Kletzander wrote:
On Fri, Jul 17, 2015 at 02:27:45PM +0300, Pavel Fedin wrote:
Here we assume that if qemu supports generic PCI host controller,
it is a part of virt machine and can be used for adding PCI devices.
In qemu this is actually a PCIe bus, so we also declare multibus
capability so that 0'th bus is specified to qemu correctly as
'pcie.0'
Signed-off-by: Pavel Fedin p.fe...@samsung.com
---
src/qemu/qemu_capabilities.c | 8
src/qemu/qemu_domain.c | 17 +
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c
b/src/qemu/qemu_capabilities.c
index d570fdd..f3486c7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2138,6 +2138,14 @@ bool
virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
return false;
}
+if (ARCH_IS_ARM(def-os.arch)) {
+/* If 'virt' supports PCI, it supports multibus.
+ * No extra conditions here for simplicity.
+ */
So every ARM qemu with the virt machine type supports both PCI and
multiqueue? How about those virt-* for which you check below.
That
might not be related, I'm just curious.
+if (STREQ(def-os.machine, virt))
+return true;
+}
+
return false;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b050a0..c7d14e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -981,7 +981,7 @@ virDomainXMLNamespace
virQEMUDriverDomainXMLNamespace = {
static int
qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
bool addDefaultUSB = true;
bool addImplicitSATA = false;
@@ -1030,12 +1030,21 @@ qemuDomainDefPostParse(virDomainDefPtr def,
break;
case VIR_ARCH_ARMV7L:
- addDefaultUSB = false;
- addDefaultMemballoon = false;
- break;
case VIR_ARCH_AARCH64:
addDefaultUSB = false;
addDefaultMemballoon = false;
+ if (STREQ(def-os.machine, virt) ||
+ STRPREFIX(def-os.machine, virt-)) {
+ virQEMUDriverPtr driver = opaque;
+
+ /* This condition is actually a (temporary) hack for
test suite which
+* does not create capabilities cache */
Few questions here. a) how temporary is this since you're not
removing it in this series? b) for what tests you need this hack and
what part of the below is the hack?
Moreover, you cannot use capabilities when defining an XML. The
emulator can change between the domain is defined and started, so you
cannot know with what emulator this will be started.
I see Michal (Cc'd) just pushed this, I probably just missed the mail
Of course I forgot, Cc'ing now.
I agree with your core statement that we should not be using the QEMU
capabilities when defining the XML. With all existing scenarios we have
been able to determine whether to add the implicit PCI controller based
on the machine type name only, because with every other QEMU arch when
doing such a major change as adding a PCI bus, they have created a new
machine type. The problem is that arm 'virt' machine type is not
stable,
it is being changed arbitrarily in new QEMU releases :-(
So AFAIK, that leaves us with 3 choices
- Never add PCI controller at time the XML is defined, on the basis
that we have to be conservative in what we add to cope with old QEMU
- Always add PCI controller at time the XML is defined, on the basis
that most people will have new enough QEMU because ARM 'virt'
machine
type is very much still in development, so no one will serously
stick
with the older QEMU versions which lack PCI.
- Use the capabilities in XML post-parse to conditionally add the
PCI controller. This is what was currently merged
I don't think option 1 makes much sense as it'll harm ARM arch forever
more, to cope with QEMU versions that will almost never be used in
practice.
I'd be inclined to go with option 2, and then if any PCI devices are
actually used with the guest, check the capability at start time when
we are doing auto-address assignment.
Another option: add versioned 'virt' machine types to the next qemu
release
(like virt-2.5 etc.), and key libvirt enabling pci off of that.
_Eventually_ we are going to need versioned 'virt' machine types for
migration
compatibility like we already do with x86 -M pc-*. Might as well make
the
change early so libvirt can actually use it.
I was also thinking about a middle ground between choices 1 and 2 from
Dan in case the machine types are not versioned any time
Re: [libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
On 10/08/15 22:29, Laine Stump wrote:
On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote:
This is a fix for commit
db488c79173b240459c7754f38c3c6af9b432970 dnsmasq main process
exits without waiting for DAD, this is dnsmasq daemon's task. So
we periodically poll the kernel using netlink and check whether
there are any IPv6 addresses assigned to bridge which have
'tentative' state. After DAD is finished, execution continues. I
guess that is what dnsmasq was assumed to do.
Since the comments in our code imply that dnsmasq should be waiting
for DAD to complete prior to daemonizing, before pushing a fix like
this I'd like to find out from the dnsmasq folks if we are
erroneously relying on nonexistent dnsmasq behavior, or if maybe
there is a bug in some version of dnsmasq.
Simon (or other dnsmasq people) - when dnsmasq is run with
enable-ra, does it make sure it completes DAD prior to
daemonizing? Or does libvirt need to do this extra polling to
assure that DAD has completed? (or maybe there's some other config
parameter we need to add?)
Dnsmasq doesn't wait for DAD to complete before returning. Internally,
it know is DAD is still happening on an interface, as it needs to
delay calling bind() until after it's complete. It would, therefore be
relatively simple to add this behaviour, but it's not a complete
solution, since new interfaces can appear _after_ dnsmasq has
completed start-up.
Having libvirt do its own checks whenever it creates an interface
might therefore be a cleaner way of doing things, but I don't have an
objection to changing dnsmasq behaviour if there's a good reason why
that's not sensible.
Cheers,
Simon.
--- Difference to v2: Moved to virnetdev.
src/libvirt_private.syms| 1 + src/network/bridge_driver.c |
35 +- src/util/virnetdev.c| 160
src/util/virnetdev.h
| 2 + 4 files changed, 197 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0517c24..fa9e1c1 100644 --- a/src/libvirt_private.syms +++
b/src/libvirt_private.syms @@ -1776,6 +1776,7 @@
virNetDevSetRcvMulti; virNetDevSetupControl; virNetDevSysfsFile;
virNetDevValidateConfig; +virNetDevWaitDadFinish;
# util/virnetdevbandwidth.h diff --git
a/src/network/bridge_driver.c b/src/network/bridge_driver.c index
3d6721b..2172a3d 100644 --- a/src/network/bridge_driver.c +++
b/src/network/bridge_driver.c @@ -2026,6 +2026,33 @@
networkAddRouteToBridge(virNetworkObjPtr network, }
static int +networkWaitDadFinish(virNetworkObjPtr network) +{ +
virNetworkIpDefPtr ipdef; +virSocketAddrPtr *addrs = NULL; +
size_t i; +int ret; +for (i = 0; + (ipdef =
virNetworkDefGetIpByIndex(network-def, AF_INET6, i)); +
i++) {} + +if (i == 0) +return 0; +if
(VIR_ALLOC_N(addrs, i)) +return -1; + +for (i = 0; +
(ipdef = virNetworkDefGetIpByIndex(network-def, AF_INET6, i)); +
i++) { +addrs[i] = ipdef-address; +} + +ret =
virNetDevWaitDadFinish(addrs, i); +VIR_FREE(addrs); +
return ret; +} + +static int
networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
virNetworkObjPtr network) { @@ -2159,7 +2186,13 @@
networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if
(v6present networkStartRadvd(driver, network) 0) goto err4;
-/* DAD has happened (dnsmasq waits for it), dnsmasq is now
bound to the +/* dnsmasq main process does not wait for DAD
to complete, + * so we need to wait for it ourselves. +
*/ +if (v6present networkWaitDadFinish(network) 0) +
goto err4; + +/* DAD has happened, dnsmasq is now bound to
the * bridge's IPv6 address, so we can now set the dummy tun
down. */ if (tapfd = 0) { diff --git a/src/util/virnetdev.c
b/src/util/virnetdev.c index 1e20789..c81342a 100644 ---
a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -96,6 +96,7
@@ VIR_LOG_INIT(util.netdev); # define
FEATURE_BIT_IS_SET(blocks, index, field)\
(FEATURE_WORD(blocks, index, field) FEATURE_FIELD_FLAG(index))
#endif +# define IP_BUF_SIZE 4096
typedef enum { VIR_MCAST_TYPE_INDEX_TOKEN, @@ -1219,6 +1220,103
@@ int virNetDevClearIPAddress(const char *ifname, return ret; }
+/* return whether there is a known address with 'tentative' flag
set */ +static int +virNetDevParseDadStatus(struct nlmsghdr *nlh,
int len, +virSocketAddrPtr *addrs, size_t
count) +{ +struct ifaddrmsg *ifaddrmsg_ptr; +unsigned int
ifaddrmsg_len; +struct rtattr *rtattr_ptr; +size_t i; +
struct in6_addr *addr; +for (; NLMSG_OK(nlh, len); nlh =
NLMSG_NEXT(nlh, len)) { +if (NLMSG_PAYLOAD(nlh, 0)
sizeof(struct ifaddrmsg)) { +/* Message without
payload is the last one. */ +break; +} + +
ifaddrmsg_ptr = (struct ifaddrmsg *)NLMSG_DATA(nlh); +if
Re: [libvirt] [PATCH 2/6] rpc: Introduce new elements 'id' and 'name' to virnetserver structure
On Tue, Aug 11, 2015 at 09:58:57AM +0200, Erik Skultety wrote:
By adding these elements, we'll be able to represent the servers on
client side. This is merely because when listing clients or managing
clients, it would be convenient to know which server they're connected
to. Also reflect this change in virnetdaemontest as well.
---
daemon/libvirtd.c | 2 ++
src/locking/lock_daemon.c | 2 +-
src/lxc/lxc_controller.c | 2 +-
src/rpc/virnetdaemon.c| 1 +
src/rpc/virnetserver.c| 17 -
src/rpc/virnetserver.h| 1 +
tests/virnetdaemondata/input-data-admin-nomdns.json | 2 ++
tests/virnetdaemondata/input-data-anon-clients.json | 1 +
tests/virnetdaemondata/input-data-initial-nomdns.json | 1 +
tests/virnetdaemondata/input-data-initial.json| 1 +
tests/virnetdaemontest.c | 2 +-
11 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 250094b..88ad753 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1390,6 +1390,7 @@ int main(int argc, char **argv) {
config-keepalive_interval,
config-keepalive_count,
config-mdns_adv ? config-mdns_name : NULL,
+default,
You're using default here, ...
remoteClientInitHook,
NULL,
remoteClientFreeFunc,
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index c035024..6e67620 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -151,7 +151,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool
privileged)
if (!(lockd-srv = virNetServerNew(1, 1, 0, config-max_clients,
config-max_clients, -1, 0,
- NULL,
+ NULL, virlockd,
... but virlockd for the virtlockd (notice your missing 't'). We
should be consistent with that, so either use default for all
default ones or name them based on the binary.
diff --git a/tests/virnetdaemondata/input-data-initial.json
b/tests/virnetdaemondata/input-data-initial.json
index 7956225..d1c801d 100644
--- a/tests/virnetdaemondata/input-data-initial.json
+++ b/tests/virnetdaemondata/input-data-initial.json
@@ -7,6 +7,7 @@
keepaliveCount: 5,
keepaliveRequired: true,
mdnsGroupName: libvirtTest,
+name: libvirtTestServer,
services: [
{
auth: 0,
Input data should not have added any field. Read the README in this
directory. The input*.json files are what the real binary will read
in real life (not tests), so that's exactly what we need to test for.
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 80b5588..ae48cbb 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -427,11 +436,17 @@ virNetServerPtr
virNetServerNewPostExecRestart(virJSONValuePtr object,
goto error;
}
+if (!(serverName = virJSONValueObjectGetString(object, name))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(Missing server name in JSON document));
+goto error;
+}
+
And that's why we must *not* fail here. I would rather you add a name
of the server to the function being called here. That name will be
used in case there is no server name in the input JSON file.
It's also easy to test in virnetdaemontest -- you add different
default name in the parameter and leave different one in the file.
Those files without the name will have the default one in output and
the new one will have the same one in output.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 16/20] cpu: Add POWER8NVL information to CPU map XML
On Mon, Aug 10, 2015 at 10:55:58 +0200, Andrea Bolognani wrote: This is yet another variation of POWER8. The PVR information comes from arch/powerpc/kernel/cputable.c in the Linux kernel tree. --- src/cpu/cpu_map.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 0895ada..d2469ee 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1372,6 +1372,7 @@ model name='POWER8' vendor name='IBM'/ pvr value='0x004b' mask='0x'/ + pvr value='0x004c' mask='0x'/ pvr value='0x004d' mask='0x'/ /model ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/9] virNetDevBandwidthUpdateRate: turn class_id into integer
On 11.08.2015 02:49, John Ferlan wrote: On 08/03/2015 02:39 AM, Michal Privoznik wrote: This is no functional change. It's just that later in the series we will need to pass class_id as an integer. Signed-off-by: Michal Privoznik mpriv...@redhat.com --- src/network/bridge_driver.c | 4 ++-- src/util/virnetdevbandwidth.c | 10 +++--- src/util/virnetdevbandwidth.h | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) something that dawned on me reading later patches... diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 6ae0877..91201ae 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -638,7 +638,8 @@ virNetDevBandwidthUnplug(const char *brname, /** * virNetDevBandwidthUpdateRate: * @ifname: interface name - * @classid: ID of class to update + * @id: unique identifier (MUST be greater than 2) ^^^ Comment says MUST be 2, but there's no check/error... Also I note 2 is passed often... Yeah, this is basically my personal note. In fact, it's not valid at all and I will remove it. The intent was that in the QoS tree that is created on the bridge, class/leaf #2 has a specific purpose = all nonguaranteed traffic goes through it. Therefore it should be updated with higher caution than anything else. But this comment is just misleading. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] virsh: Rework parseRateStr
On 11.08.2015 03:09, John Ferlan wrote:
On 08/03/2015 02:39 AM, Michal Privoznik wrote:
The function is used to parse a tuple delimited by commas into
virNetDevBandwidth structure. So far only three out of fore
fields are supported: average, peak and burst. The single missing
field is floor. Well, the parsing works, but I think we can do
better. Especially when we will need to parse floor too in very
close future.
Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
tools/virsh-domain.c | 80
++--
1 file changed, 47 insertions(+), 33 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a61656f..bb40ddd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -865,36 +865,58 @@ static const vshCmdOptDef opts_attach_interface[] = {
/* parse inbound and outbound which are in the format of
* 'average,peak,burst', in which peak and burst are optional,
* thus 'average,,burst' and 'average,peak' are also legal. */
-static int parseRateStr(const char *rateStr, virNetDevBandwidthRatePtr rate)
+static int parseRateStr(vshControl *ctl,
+const char *rateStr,
+virNetDevBandwidthRatePtr rate)
{
-const char *average = NULL;
-char *peak = NULL, *burst = NULL;
+char *token;
+char *next;
+char *saveptr;
My compiler complained about uninitialized value here especially when
used with strtok_r (setting = NULL pacifies compiler).
Interesting since I want strtok_r to initialize the variable to a value.
Whatever, initializing to NULL - I can live with that.
ACK with the adjustment.
John
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers
This started out as a fix for a crash reported in IRC and on libvir-list:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
but as I examined the existing code I found so many small nits to pick
in surrounding functions that I decided to fix them all in this patch.
The most important fix here is that virNetDevGetFeatures was creating
a struct ethtool_gfeatures object as a local on the stack and,
although the struct is defined with 0 elements in its features array,
we were telling the ethtool ioctl that we had made space for 2
elements. This led to a crash, as outlined in the email above. The fix
for this is to allocate the memory for the ethtool_gfeatures object
using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type
ethtool_get_features_block
Beyond that crash fixer, the following fixups were made to the
hierarchy of functions between virNetDevGetFeatures() and
virNetDevSendEthtoolIoctl():
* macros used to examine the gfeatures bits were renamed from
FEATURE_* to GFEATURE_*
virNetDevSentEthtoolIoctl():
* no need to initialize sock to -1, since it is always set at the top
of the function.
* remove VIR_DEBUG log (and subsequent *skipping* of error log!) when
errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were
ever encountered, this would have been *very* problematic, as it
would have led to one of those situations where virsh reports an
error was encountered but the cause is unknown (or whatever the
message is when we have an error but no log message).
* always call VIR_FORCE_CLOSE(sock) since we know that sock is either
a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip).
virNetDevFeatureAvailable()
* simplify it - no need for ret.
* follow libvirt convention of checking for bobLobLaw(lawblog) 0
instead of !bobLobLaw(lawblog).
virNetDevGFeatureAvailable()
* eliminate this function, as it was ill-conceived (it really was only
checking for one gfeature (TX_UDP_TNL), not *any* gfeature.
virNetDevGetFeatures()
* move all data tables (struct elem) out of the function so that they
will be statics instead of taking up space on the stack.
* remove pointless/incorrect initialization of i = -1.
* remove unnecessary static initialization of struct ethtool_value cmd
* g_cmd is now a pointer instead of automatic
* use libvirt convention of checking return from
virNetDevFeatureAvailable() 0, instead of
!virNetDevFeatureAvailable(), and immediately return to caller
with an error when virNetDevFeatureAvailable() returns an error
(previously it was ignored).
* remove superfluous size_t j, and just re-use i instead.
* runtime allocation/free of proper size object for ethtoolfeatures as
described above.
* directly call virNetDevSentEthtoolIoctl() instead of now defunct
virNetDevGFeatureAvailable().
---
V2: I had been looking for something like VIR_ALLOC_VAR() when writing
the patch originally, but somehow missed it, and did an ugly hack with
VIR_ALLOC_N and a union. In this version I clean that up and use
VIR_ALLOC_VAR() instead.
NB: This patch does *not* attempt to determine the proper number of
elements for the gfeature array at runtime, as proposed in this patch:
https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
since that wasn't the cause of the crash. I'll leave it up to Moshe to
repost that patch rebased around this one (or whatever ends up being
pushed) if he thinks there is value to it.
Also - as I mentioned in earlier mail in response to the crash, I
noticed when looking into the gfeatures ethtool code that it looks to
me like TX_UDP_TNL should actually be 26 rather than 25, but I may be
missing something. Moshe - can you either confirm or deny that? Where
did you get the value 25 from?
src/util/virnetdev.c | 177 +++
1 file changed, 79 insertions(+), 98 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..05fbff5 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -91,10 +91,10 @@ VIR_LOG_INIT(util.netdev);
#if HAVE_DECL_ETHTOOL_GFEATURES
# define TX_UDP_TNL 25
# define GFEATURES_SIZE 2
-# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
-# define FEATURE_FIELD_FLAG(index) (1U (index) % 32U)
-# define FEATURE_BIT_IS_SET(blocks, index, field)\
-(FEATURE_WORD(blocks, index, field) FEATURE_FIELD_FLAG(index))
+# define GFEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
+# define GFEATURE_FIELD_FLAG(index) (1U (index) % 32U)
+# define GFEATURE_BIT_IS_SET(blocks, index, field)\
+(GFEATURE_WORD(blocks, index, field) GFEATURE_FIELD_FLAG(index))
#endif
typedef enum {
@@ -3032,11 +3032,10 @@ static int
virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
{
int ret = -1;
-int sock = -1;
+int sock;
virIfreq ifr;
-sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
-if (sock 0) {
+if
Re: [libvirt] [PATCH v3 13/20] cpu: Support multiple PVRs in the ppc64 driver
On Mon, Aug 10, 2015 at 10:55:55 +0200, Andrea Bolognani wrote:
This will allow us to perform PVR matching more broadly, eg. consider
both POWER8 and POWER8E CPUs to be the same even though they have
different PVR values.
---
src/cpu/cpu_ppc64.c | 72
src/cpu/cpu_ppc64_data.h | 8 +-
2 files changed, 67 insertions(+), 13 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 6c78ab8..db25f27 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -60,6 +60,10 @@ struct ppc64_map {
static void
ppc64DataFree(virCPUppc64Data *data)
{
+if (!data)
+return;
+
+VIR_FREE(data-pvr);
VIR_FREE(data);
}
@@ -67,13 +71,24 @@ static virCPUppc64Data *
ppc64DataCopy(const virCPUppc64Data *data)
{
virCPUppc64Data *copy;
+size_t i;
if (VIR_ALLOC(copy) 0)
-return NULL;
+goto error;
+
+if (VIR_ALLOC_N(copy-pvr, data-len) 0)
+goto error;
+
+copy-len = data-len;
-copy-pvr = data-pvr;
+for (i = 0; i data-len; i++)
+copy-pvr[i].value = data-pvr[i].value;
return copy;
+
+ error:
+ppc64DataFree(copy);
+return NULL;
}
static void
@@ -159,12 +174,14 @@ ppc64ModelFindPVR(const struct ppc64_map *map,
uint32_t pvr)
{
struct ppc64_model *model;
+size_t i;
model = map-models;
while (model) {
-if (model-data-pvr == pvr)
-return model;
-
+for (i = 0; i model-data-len; i++) {
+if (model-data-pvr[i].value == pvr)
+return model;
+}
model = model-next;
}
@@ -257,8 +274,16 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt,
struct ppc64_map *map)
{
struct ppc64_model *model;
+xmlNodePtr *nodes = NULL;
+xmlNodePtr bookmark;
char *vendor = NULL;
unsigned long pvr;
+size_t i;
+int n;
+
+/* Save the node the context was pointing to, as we're going
+ * to change it later. It's going to be restored on exit */
+bookmark = ctxt-node;
if (VIR_ALLOC(model) 0)
return -1;
@@ -298,14 +323,30 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt,
}
}
-if (!virXPathBoolean(boolean(./pvr), ctxt) ||
-virXPathULongHex(string(./pvr/@value), ctxt, pvr) 0) {
+if ((n = virXPathNodeSet(./pvr, ctxt, nodes)) = 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _(Missing or invalid PVR value in CPU model %s),
+ _(Missing PVR information for CPU model %s),
model-name);
goto ignore;
}
-model-data-pvr = pvr;
+
+if (VIR_ALLOC_N(model-data-pvr, n) 0)
+goto ignore;
+
+model-data-len = n;
+
+for (i = 0; i n; i++) {
+ctxt-node = nodes[i];
+
+if (!virXPathBoolean(boolean(./@value), ctxt) ||
+virXPathULongHex(string(./@value), ctxt, pvr) 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _(Missing or invalid PVR value in CPU model
%s),
+ model-name);
+goto ignore;
Wrong indentation, s/^// in the 4 lines above. ACK once it's fixed.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert LXC: show used memory as 0 when domain is not active
On Tue, Aug 11, 2015 at 10:25:19AM +0200, Peter Krempa wrote: On Tue, Aug 11, 2015 at 09:18:23 +0100, Daniel Berrange wrote: On Mon, Aug 10, 2015 at 12:49:55PM -0600, Jim Fehlig wrote: This reverts commit 1ce7c1d20cfd5afb26d2dbc88201085d52415d0e, which introduced a significant semantic change to the virDomainGetInfo() API. Additionally, the change was only made to 2 of the 15 virt drivers. Conflicts: src/qemu/qemu_driver.c Signed-off-by: Jim Fehlig jfeh...@suse.com --- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 12 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) ACK I guess we should probably do this in the stable branch too, since I think it made it into the most recent release. Erm, the change is out for a while now: git desc 1ce7c1d20cfd5afb26d2dbc88201085d52415d0e v1.2.6-225-g1ce7c1d Oh yes, so it is. I'm still inclined to say we should be reverting it as I think it is wrong. The change was based on the misleading field name shown by virsh. The info-memory field shows the current balloon target, and conceptually this should be equal to the max memory if the ballooon driver is not active. As such I think it should be equal to max memory if shutoff too. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/6] admin: Usage example of the new server listing API
On Tue, Aug 11, 2015 at 09:59:01AM +0200, Erik Skultety wrote:
Not to be actually pushed since majority of this example will be merged
into virt-admin once it's ready, i.e. virsh splitting series is merged,
but might be good to just see the API's working.
---
I would say please note in the subject that this is not to be
pushed, but since even I can screw that up and push it afterwards, it
seems to be said by the wrong person :)
.gitignore | 1 +
Makefile.am | 2 +-
configure.ac | 1 +
examples/admin/Makefile.am | 25 +++
examples/admin/listservers.c | 73
5 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 examples/admin/Makefile.am
create mode 100644 examples/admin/listservers.c
diff --git a/.gitignore b/.gitignore
index 0b40f4a..325f04f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
/docs/libvirt-refs.xml
/docs/search.php
/docs/todo.html.in
+/examples/admin/listservers
/examples/object-events/event-test
/examples/dominfo/info1
/examples/domsuspend/suspend
diff --git a/Makefile.am b/Makefile.am
index 91b943b..c14229e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -24,7 +24,7 @@ SUBDIRS = . gnulib/lib include src daemon tools docs
gnulib/tests \
examples/dominfo examples/domsuspend examples/apparmor \
examples/xml/nwfilter examples/openauth examples/systemtap \
tools/wireshark examples/dommigrate \
- examples/lxcconvert examples/domtop
+ examples/lxcconvert examples/domtop examples/admin
ACLOCAL_AMFLAGS = -I m4
diff --git a/configure.ac b/configure.ac
index 46c80ce..35c8cd9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2805,6 +2805,7 @@ AC_CONFIG_FILES([\
examples/systemtap/Makefile \
examples/xml/nwfilter/Makefile \
examples/lxcconvert/Makefile \
+examples/admin/Makefile \
tools/wireshark/Makefile \
tools/wireshark/src/Makefile])
AC_OUTPUT
diff --git a/examples/admin/Makefile.am b/examples/admin/Makefile.am
new file mode 100644
index 000..8373132
--- /dev/null
+++ b/examples/admin/Makefile.am
@@ -0,0 +1,25 @@
+## Copyright (C) 2005-2015 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
+## http://www.gnu.org/licenses/.
+
+INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include
+LDADDS = $(STATIC_BINARIES) $(WARN_CFLAGS) \
+$(top_builddir)/src/libvirt-admin.la $(COVERAGE_LDFLAGS)
+
+noinst_PROGRAMS=listservers
+
+listservers_SOURCES=listservers.c
+listservers_LDFLAGS=
+listservers_LDADD= $(LDADDS)
diff --git a/examples/admin/listservers.c b/examples/admin/listservers.c
new file mode 100644
index 000..6f11d8d
--- /dev/null
+++ b/examples/admin/listservers.c
@@ -0,0 +1,73 @@
+/*
+ * listservers.c: Demo program to show listing of available servers
+ *
+ * Copyright (C) 2015 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
+ * http://www.gnu.org/licenses/.
+ *
+ * Author: Erik Skultety eskul...@redhat.com
+ */
+
+#include stdio.h
+#include stdlib.h
+#include libvirt/libvirt-admin.h
+
+static int
+listDaemonServers(void)
+{
+int ret = -1;
+virAdmConnectPtr conn = NULL;
+virAdmServerPtr *srvs = NULL;
+int nsrvs = 0;
+size_t i;
+
+/* Connect to an admin server on a specific daemon, NULL in this case means
+ * connect to libvirtd UNIX socket
+ */
+if (!(conn = virAdmConnectOpen(NULL, 0))) {
+fprintf(stderr, Failed to connect to the admin server\n);
There will be error printed by the default error handler, no need to
write out another error.
+goto cleanup;
+}
+
+/* Obtain a list of available servers on the daemon */
+if ((nsrvs = virAdmConnectListServers(conn, srvs, 0)) 0) {
+fprintf(stderr, Failed
Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel
On 08/08/2015 05:34 AM, Moshe Levi wrote:
This patch add virNetDevGetGFeaturesSize to get the supported
gfeature size from the kernel
---
This is interesting/possibly useful, but it doesn't fix the crash that
users are experiencing. Here is a patch that should fix the crash:
https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
I would rather have that patch pushed before this one (which will mean
rebasing and resolving some merge conflicts).
src/util/virnetdev.c | 60 ++---
1 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..3fdf4bb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -89,8 +89,10 @@ VIR_LOG_INIT(util.netdev);
#define RESOURCE_FILE_LEN 4096
#if HAVE_DECL_ETHTOOL_GFEATURES
+#define ETH_GSTRING_LEN 32
Where does the 32 come from? Is it from ETH_GSTRING_LEN (defined in
ethtool.h)? If so, why not just use that directly instead of #defining a
new one?
# define TX_UDP_TNL 25
I'll keep asking this every time I encounter it until I get a response -
when I look at the ethtool source files in the kernel, it looks to me
like the index for this capability should be 26 and not 25. Where/how
did you arrive at the value of 25?
-# define GFEATURES_SIZE 2
+# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+# define FEATURE_BITS_TO_BLOCKS(n_bits) DIV_ROUND_UP(n_bits, 32U)
# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field)
# define FEATURE_FIELD_FLAG(index) (1U (index) % 32U)
# define FEATURE_BIT_IS_SET(blocks, index, field)\
@@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char *ifname, struct
ethtool_gfeatures *cmd)
ret = FEATURE_BIT_IS_SET(cmd-features, TX_UDP_TNL, active);
return ret;
}
+/**
+ * virNetDevGetGFeaturesSize
+ * This function return the number of gfeatures supported in
+ * the kernel
+ *
+ * @ifname: name of the interface
+ *
+ * Returns gfeature size on success, and 0 on failure or not supported.
+ */
+ static int
+virNetDevGetGFeaturesSize(const char *ifname)
+{
+struct {
+struct ethtool_sset_info hdr;
+uint32_t buf[1];
+} sset_info;
+struct ethtool_gstrings *strings;
+uint32_t size = 0;
+
+sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
+sset_info.hdr.reserved = 0;
+sset_info.hdr.sset_mask = 1ULL ETH_SS_FEATURES;
+
+if (virNetDevSendEthtoolIoctl(ifname, sset_info) == -1)
+return size;
virNetDevSendEthtoolIoctl() logs an error message, but it looks like you
want for an error to be swallowed here (just returning size = 0). If
that's the case then virNetDevSendEthtoolIoctl() needs to be reworked to
not log errors, then every caller to it needs to log the error.
+size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;
+
+if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) +
size * ETH_GSTRING_LEN)) {
+size = 0;
+return size;
+}
+
+strings-cmd = ETHTOOL_GSTRINGS;
+strings-string_set = ETH_SS_FEATURES;
+strings-len = size;
+if (size != 0 virNetDevSendEthtoolIoctl(ifname, strings) == -1) {
+size = 0;
+goto cleanup;
+}
+
+size = FEATURE_BITS_TO_BLOCKS(size);
+
+ cleanup:
+VIR_FREE(strings);
+return size;
+}
+
# endif
@@ -3189,9 +3238,12 @@ virNetDevGetFeatures(const char *ifname,
# if HAVE_DECL_ETHTOOL_GFEATURES
g_cmd.cmd = ETHTOOL_GFEATURES;
-g_cmd.size = GFEATURES_SIZE;
-if (virNetDevGFeatureAvailable(ifname, g_cmd))
-ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+g_cmd.size = virNetDevGetGFeaturesSize(ifname);
+if (g_cmd.size == 0)
+VIR_DEBUG(GFeatures unsupported or failure in getting GFeatures
size on ifname %s, ifname);
Should this failure really be ignored? Or should it lead to failure of
the entire operation?
+else
+if (virNetDevGFeatureAvailable(ifname, g_cmd))
+ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
# endif
if (virNetDevRDMAFeature(ifname, out) 0)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/20] tests: Improve result handling in cpuTestGuestData()
On Mon, Aug 10, 2015 at 10:55:49 +0200, Andrea Bolognani wrote: A test is considered successful if the obtained result matches the expected result: if that's not the case, whether because a test that was expected to succeed failed or because a test that was supposed to fail succeeded, then something's not right and we want the user to know about this. On the other hand, if a failure that's unrelated to the bits we're testing occurs, then the user should be notified even if the test was expected to fail. Use different values to tell these two situations apart. Fix a test case that was wrongly expected to fail as well. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] admin: Introduce adminDaemonConnectListServers API
On Tue, Aug 11, 2015 at 09:59:00AM +0200, Erik Skultety wrote:
This is the first API to the admin interface. This particular API is
a convenience API, i.e. when managing clients connected to daemon's servers,
we should know (convenience) which server the specific client is connected to.
This implies a client-side representation of a server along with a basic
API to let the administrating client know what servers are actually
available on the daemon.
---
daemon/Makefile.am | 2 +-
daemon/admin.c | 57 +
daemon/admin_server.c | 83 +++
daemon/admin_server.h | 33
include/libvirt/libvirt-admin.h | 12 +++
src/admin/admin_protocol.x | 27 ++-
src/datatypes.c | 35
src/datatypes.h | 35
src/libvirt-admin.c | 173
src/libvirt_admin.syms | 4 +
src/rpc/virnetdaemon.c | 15
src/rpc/virnetdaemon.h | 2 +
src/rpc/virnetserver.c | 24 ++
src/rpc/virnetserver.h | 3 +
14 files changed, 503 insertions(+), 2 deletions(-)
create mode 100644 daemon/admin_server.c
create mode 100644 daemon/admin_server.h
I would probably split the introduction of the virAdmServer structure
into separate patch. You also forgot to update the
admin_protocol-structs, I guess.
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 5637f5a..d4dd4ac 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -128,7 +128,7 @@ libvirtd_conf_la_LIBADD = $(LIBXML_LIBS)
noinst_LTLIBRARIES += libvirtd_admin.la
libvirtd_admin_la_SOURCES = \
- admin.c admin.h
+ admin.c admin.h admin_server.c admin_server.h
libvirtd_admin_la_CFLAGS = \
$(AM_CFLAGS)\
diff --git a/daemon/admin.c b/daemon/admin.c
index ef404a9..d765231 100644
--- a/daemon/admin.c
+++ b/daemon/admin.c
@@ -28,6 +28,7 @@
#include admin_protocol.h
#include admin.h
+#include admin_server.h
#include datatypes.h
#include viralloc.h
#include virerror.h
@@ -77,6 +78,16 @@ remoteAdmClientInitHook(virNetServerClientPtr client
ATTRIBUTE_UNUSED,
return priv;
}
+/* Helpers */
+
+static void
+make_nonnull_server(admin_nonnull_server *srv_dst,
+virAdmServerPtr srv_src)
Indentation's off.
+{
+srv_dst-id = srv_src-id;
+ignore_value(VIR_STRDUP_QUIET(srv_dst-name, srv_src-name));
+}
+
/* Functions */
static int
adminDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
@@ -114,4 +125,50 @@ adminDispatchConnectClose(virNetServerPtr server
ATTRIBUTE_UNUSED,
return 0;
}
+
+static int
+adminDispatchConnectListServers(virNetServerPtr server ATTRIBUTE_UNUSED,
+virNetServerClientPtr client,
+virNetMessagePtr msg ATTRIBUTE_UNUSED,
+virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+admin_connect_list_servers_args *args,
+admin_connect_list_servers_ret *ret)
+{
+virAdmServerPtr *servers = NULL;
+int nservers = 0;
+int rv = -1;
+size_t i;
+struct daemonAdmClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+
+if ((nservers =
+adminDaemonConnectListServers(priv-dmn,
+ args-need_results ? servers : NULL,
+ args-flags)) 0)
+goto cleanup;
+
+if (servers nservers) {
+if (VIR_ALLOC_N(ret-servers.servers_val, nservers) 0)
+goto cleanup;
+
+ret-servers.servers_len = nservers;
+for (i = 0; i nservers; i++)
+make_nonnull_server(ret-servers.servers_val + i, servers[i]);
+} else {
+ret-servers.servers_len = 0;
+ret-servers.servers_val = NULL;
+}
+
+ret-ret = nservers;
+rv = 0;
+
+ cleanup:
+if (rv 0)
+virNetMessageSaveError(rerr);
+if (servers nservers 0)
+for (i = 0; i nservers; i++)
+virObjectUnref(servers[i]);
+VIR_FREE(servers);
+return rv;
+}
#include admin_dispatch.h
diff --git a/daemon/admin_server.c b/daemon/admin_server.c
new file mode 100644
index 000..9af94ee
--- /dev/null
+++ b/daemon/admin_server.c
@@ -0,0 +1,83 @@
+/*
+ * admin_server.c: admin methods to manage daemons and clients
+ *
+ * Copyright (C) 2014-2015 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
+ *
Re: [libvirt] Buffer overflow error when starting libvirtd
On 08/11/2015 01:30 AM, Ján Tomko wrote: On Mon, Aug 10, 2015 at 11:21:47PM -0400, Willard Dennis wrote: Compiled libvirt 1.2.18 on Ubuntu 12.04.5 (installed over 12.04 libvirt-bin 0.9.9 package files); when I try to start libvirtd, I get the following error / backtrace: $ sudo /usr/sbin/libvirtd *** stack smashing detected ***: /usr/sbin/libvirtd terminated === Backtrace: = /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f02fe6d3e57] /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x0)[0x7f02fe6d3e20] /usr/lib/libvirt.so.0(+0xaa359)[0x7f02fec4f359] /usr/lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0x99fe)[0x7f02f1fc59fe] /usr/lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xa672)[0x7f02f1fc6672] /usr/lib/libvirt.so.0(virStateInitialize+0xaf)[0x7f02fed1cd2f] /usr/sbin/libvirtd(+0x17d80)[0x7f02ff98dd80] /usr/lib/libvirt.so.0(+0xd25d2)[0x7f02fec775d2] /lib/x86_64-linux-gnu/libpthread.so.0(+0x7e9a)[0x7f02fe98fe9a] /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7f02fe6bd38d] Not sure if this may be a bug with 1.2.18, or a fault with my configure/compile/install process; please advise... This has been reported last week: https://www.redhat.com/archives/libvir-list/2015-August/msg00163.html And here is the patch proposed to fix it, waiting on review: https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html Willard - If you're able to build libvirt and test that patch, it would be greatly appreciated. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel
-Original Message-
From: sendmail [mailto:justsendmailnothinge...@gmail.com] On Behalf Of
Laine Stump
Sent: Tuesday, August 11, 2015 9:27 AM
To: libvir-list@redhat.com
Cc: Moshe Levi
Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to
running kernel
On 08/08/2015 05:34 AM, Moshe Levi wrote:
This patch add virNetDevGetGFeaturesSize to get the supported gfeature
size from the kernel
---
This is interesting/possibly useful, but it doesn't fix the crash that users
are
experiencing. Here is a patch that should fix the crash:
https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
I would rather have that patch pushed before this one (which will mean
rebasing and resolving some merge conflicts).
Ok I will rebase once you patch is merged.
src/util/virnetdev.c | 60
++---
1 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index
1e20789..3fdf4bb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -89,8 +89,10 @@ VIR_LOG_INIT(util.netdev);
#define RESOURCE_FILE_LEN 4096
#if HAVE_DECL_ETHTOOL_GFEATURES
+#define ETH_GSTRING_LEN 32
Where does the 32 come from? Is it from ETH_GSTRING_LEN (defined in
ethtool.h)? If so, why not just use that directly instead of #defining a new
one?
It is define in ethtool-copy.h
# define TX_UDP_TNL 25
I'll keep asking this every time I encounter it until I get a response - when
I
look at the ethtool source files in the kernel, it looks to me like the index
for
this capability should be 26 and not 25. Where/how did you arrive at the
value of 25?
I printed the value when I run ethtool command
moshe index 25
tx-udp_tnl-segmentation: on
moshe index 26
tx-mpls-segmentation: off [fixed]
ethtool modified code
static void dump_one_feature(const char *indent, const char *name,
const struct feature_state *state,
const struct feature_state *ref_state,
u32 index)
{
if (ref_state
!(FEATURE_BIT_IS_SET(state-features.features, index, active) ^
FEATURE_BIT_IS_SET(ref_state-features.features, index, active)))
return;
printf(moshe index %d\n,index);
printf(%s%s: %s%s\n,
indent, name,
FEATURE_BIT_IS_SET(state-features.features, index, active) ?
on : off,
(!FEATURE_BIT_IS_SET(state-features.features, index, available)
|| FEATURE_BIT_IS_SET(state-features.features, index,
never_changed))
? [fixed]
: (FEATURE_BIT_IS_SET(state-features.features, index, requested)
^ FEATURE_BIT_IS_SET(state-features.features, index, active))
? (FEATURE_BIT_IS_SET(state-features.features, index, requested)
? [requested on] : [requested off])
: );
}
Also this how I got the GFEATURES_SIZE 2
-# define GFEATURES_SIZE 2
+# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+# define FEATURE_BITS_TO_BLOCKS(n_bits) DIV_ROUND_UP(n_bits,
32U)
# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) /
32U].field)
# define FEATURE_FIELD_FLAG(index) (1U (index) % 32U)
# define FEATURE_BIT_IS_SET(blocks, index, field)\
@@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char
*ifname, struct ethtool_gfeatures *cmd)
ret = FEATURE_BIT_IS_SET(cmd-features, TX_UDP_TNL, active);
return ret;
}
+/**
+ * virNetDevGetGFeaturesSize
+ * This function return the number of gfeatures supported in
+ * the kernel
+ *
+ * @ifname: name of the interface
+ *
+ * Returns gfeature size on success, and 0 on failure or not supported.
+ */
+ static int
+virNetDevGetGFeaturesSize(const char *ifname) {
+struct {
+struct ethtool_sset_info hdr;
+uint32_t buf[1];
+} sset_info;
+struct ethtool_gstrings *strings;
+uint32_t size = 0;
+
+sset_info.hdr.cmd = ETHTOOL_GSSET_INFO;
+sset_info.hdr.reserved = 0;
+sset_info.hdr.sset_mask = 1ULL ETH_SS_FEATURES;
+
+if (virNetDevSendEthtoolIoctl(ifname, sset_info) == -1)
+return size;
virNetDevSendEthtoolIoctl() logs an error message, but it looks like you want
for an error to be swallowed here (just returning size = 0). If that's the
case
then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors,
then every caller to it needs to log the error.
This was comment by John Ferlan he preferred the method will return size 0 if
not supported
or error. My previous patch which I send to him directly and not the ML return
-1 on failure.
But thinking about this again I don't want to swallow if error occur.
What do you think?
Re: [libvirt] [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers
-Original Message- From: Laine Stump [mailto:la...@laine.org] Sent: Tuesday, August 11, 2015 7:10 AM To: libvir-list@redhat.com Cc: Moshe Levi; Brian Rak; james.p.chap...@intel.com Subject: [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers This started out as a fix for a crash reported in IRC and on libvir-list: https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html but as I examined the existing code I found so many nits to pick that I just did them all. The most important fix here is that virNetDevGetFeatures was creating a struct ethtool_gfeatures object as a local on the stack and, although the struct is defined with 0 elements in its features array, we were telling the ethtool ioctl that we had made space for 2 elements. This led to a crash, as outlined in the email above. The fix for this is to allocate the memory for the ethtool_gfeatures object using VIR_ALLOC_N to a char*, giving it the length: I test it on rhel7 several time with enabling and disabling the TX_UDP_TNL sizeof(ethtool_gfeatures) + (2 * sizeof(ethtool_get_features_block) because VIR_ALLOC_N is a macro and fails when you try to typecast a pointer to char* within the invocation, I made a union that has both a char* and an ethtool_gfeatures*, and used the char* of the union for VIR_ALLOC_N, and the other for everything else. Beyond that crash fixer, the following fixups were made to the hierarchy of functions between virNetDevGetFeatures() and virNetDevSendEthtoolIoctl(): * macros used to examine the gfeatures bits were renamed from FEATURE_* to GFEATURE_* virNetDevSentEthtoolIoctl(): * no need to initialize sock to -1, since it is always set at the top of the function. * remove VIR_DEBUG log (and subsequent *skipping* of error log!) when errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were ever encountered, this would have been *very* problematic, as it would have led to one of those situations where virsh reports an error was encountered but the cause is unknown (or whatever the message is when we have an error but no log message). * always call VIR_FORCE_CLOSE(sock) since we know that sock is either a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip). virNetDevFeatureAvailable() * simplify it - no need for ret. * follow libvirt convention of checking for bobLobLaw(lawblog) 0 instead of !bobLobLaw(lawblog). virNetDevGFeatureAvailable() * eliminate this function, as it was ill-conceived (it really was only checking for one gfeature (TX_UDP_TNL), not *any* gfeature. virNetDevGetFeatures() * move all data tables (struct elem) out of the function so that they will be statics instead of taking up space on the stack. * remove pointless/incorrect initialization of i = -1. * remove unnecessary static initialization of struct ethtool_value cmd * put struct ethtool_gfeatures into a union with a char* as described above * use libvirt convention of checking return from virNetDevFeatureAvailable() 0, instead of !virNetDevFeatureAvailable(), and immediately return to caller with an error when virNetDevFeatureAvailable() returns an error (previously it was ignored). * remove superfluous size_t j, and just re-use i instead. * runtime allocation/free of proper size object for ethtoolfeatures as described above. * directly call virNetDevSentEthtoolIoctl() instead of now defunct virNetDevGFeatureAvailable(). === NB: This patch does *not* attempt to determine the proper number of elements for the gfeature array at runtime, as proposed in this patch: https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html since that wasn't the cause of the crash. I'll leave it up to Moshe to repost that patch rebased around this one (or whatever ends up being pushed) if he thinks there is value to it. Also - as I mentioned in earlier mail in response to the crash, I noticed when looking into the gfeatures ethtool code that it looks to me like TX_UDP_TNL should actually be 26 rather than 25, but I may be missing something. Moshe - can you either confirm or deny that? Where did you get the value 25 from? I answer to your question on my patch https://www.redhat.com/archives/libvir-list/2015-August/msg00389.html --- src/util/virnetdev.c | 182 --- 1 file changed, 84 insertions(+), 98 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1e20789..7f0837d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -91,10 +91,10 @@ VIR_LOG_INIT(util.netdev); #if HAVE_DECL_ETHTOOL_GFEATURES # define TX_UDP_TNL 25 # define GFEATURES_SIZE 2 -# define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field) -# define FEATURE_FIELD_FLAG(index) (1U (index) % 32U) -# define FEATURE_BIT_IS_SET(blocks, index, field)
Re: [libvirt] [PATCH v3 12/20] cpu: Align ppc64 CPU data with x86
On Mon, Aug 10, 2015 at 10:55:54 +0200, Andrea Bolognani wrote: Use a typedef instead of the plain struct and heap allocation. This will make it easier to extend the ppc64 specific CPU data later on. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/6] test: Replace tabs with spaces in input-data-admin-nomdns.json
On Tue, Aug 11, 2015 at 09:58:56AM +0200, Erik Skultety wrote: JSON data that are used to initialize tests in virnetdaemontest should be in a consistent format, i.e. not using tabs for indentation, those should be replaced by spaces. --- .../virnetdaemondata/input-data-admin-nomdns.json | 244 ++--- 1 file changed, 122 insertions(+), 122 deletions(-) I don't know how I screwed that up, but thatnks for fixing. I'm guessing this is not based on top of the keepalive_required removal that I probably screwed up the same way, right? signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert LXC: show used memory as 0 when domain is not active
On Tue, Aug 11, 2015 at 09:18:23 +0100, Daniel Berrange wrote: On Mon, Aug 10, 2015 at 12:49:55PM -0600, Jim Fehlig wrote: This reverts commit 1ce7c1d20cfd5afb26d2dbc88201085d52415d0e, which introduced a significant semantic change to the virDomainGetInfo() API. Additionally, the change was only made to 2 of the 15 virt drivers. Conflicts: src/qemu/qemu_driver.c Signed-off-by: Jim Fehlig jfeh...@suse.com --- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 12 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) ACK I guess we should probably do this in the stable branch too, since I think it made it into the most recent release. Erm, the change is out for a while now: git desc 1ce7c1d20cfd5afb26d2dbc88201085d52415d0e v1.2.6-225-g1ce7c1d Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
On Mon, Aug 10, 2015 at 07:23:07PM -0400, Cole Robinson wrote:
On 08/10/2015 11:09 AM, Daniel P. Berrange wrote:
On Thu, Aug 06, 2015 at 07:46:58PM +0200, Martin Kletzander wrote:
On Thu, Aug 06, 2015 at 06:37:41PM +0200, Martin Kletzander wrote:
On Fri, Jul 17, 2015 at 02:27:45PM +0300, Pavel Fedin wrote:
Here we assume that if qemu supports generic PCI host controller,
it is a part of virt machine and can be used for adding PCI devices.
In qemu this is actually a PCIe bus, so we also declare multibus
capability so that 0'th bus is specified to qemu correctly as 'pcie.0'
Signed-off-by: Pavel Fedin p.fe...@samsung.com
---
src/qemu/qemu_capabilities.c | 8
src/qemu/qemu_domain.c | 17 +
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d570fdd..f3486c7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2138,6 +2138,14 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr
qemuCaps,
return false;
}
+if (ARCH_IS_ARM(def-os.arch)) {
+/* If 'virt' supports PCI, it supports multibus.
+ * No extra conditions here for simplicity.
+ */
So every ARM qemu with the virt machine type supports both PCI and
multiqueue? How about those virt-* for which you check below. That
might not be related, I'm just curious.
+if (STREQ(def-os.machine, virt))
+return true;
+}
+
return false;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b050a0..c7d14e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -981,7 +981,7 @@ virDomainXMLNamespace
virQEMUDriverDomainXMLNamespace = {
static int
qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
bool addDefaultUSB = true;
bool addImplicitSATA = false;
@@ -1030,12 +1030,21 @@ qemuDomainDefPostParse(virDomainDefPtr def,
break;
case VIR_ARCH_ARMV7L:
- addDefaultUSB = false;
- addDefaultMemballoon = false;
- break;
case VIR_ARCH_AARCH64:
addDefaultUSB = false;
addDefaultMemballoon = false;
+ if (STREQ(def-os.machine, virt) ||
+ STRPREFIX(def-os.machine, virt-)) {
+ virQEMUDriverPtr driver = opaque;
+
+ /* This condition is actually a (temporary) hack for test
suite which
+* does not create capabilities cache */
Few questions here. a) how temporary is this since you're not
removing it in this series? b) for what tests you need this hack and
what part of the below is the hack?
Moreover, you cannot use capabilities when defining an XML. The
emulator can change between the domain is defined and started, so you
cannot know with what emulator this will be started.
I see Michal (Cc'd) just pushed this, I probably just missed the mail
Of course I forgot, Cc'ing now.
I agree with your core statement that we should not be using the QEMU
capabilities when defining the XML. With all existing scenarios we have
been able to determine whether to add the implicit PCI controller based
on the machine type name only, because with every other QEMU arch when
doing such a major change as adding a PCI bus, they have created a new
machine type. The problem is that arm 'virt' machine type is not stable,
it is being changed arbitrarily in new QEMU releases :-(
So AFAIK, that leaves us with 3 choices
- Never add PCI controller at time the XML is defined, on the basis
that we have to be conservative in what we add to cope with old QEMU
- Always add PCI controller at time the XML is defined, on the basis
that most people will have new enough QEMU because ARM 'virt' machine
type is very much still in development, so no one will serously stick
with the older QEMU versions which lack PCI.
- Use the capabilities in XML post-parse to conditionally add the
PCI controller. This is what was currently merged
I don't think option 1 makes much sense as it'll harm ARM arch forever
more, to cope with QEMU versions that will almost never be used in
practice.
I'd be inclined to go with option 2, and then if any PCI devices are
actually used with the guest, check the capability at start time when
we are doing auto-address assignment.
Another option: add versioned 'virt' machine types to the next qemu release
(like virt-2.5 etc.), and key libvirt enabling pci off of that.
_Eventually_ we are going to need versioned 'virt' machine types for migration
compatibility like we already do with x86 -M pc-*. Might as well make the
change early so libvirt can actually use it.
I've raised the idea of versioned 'virt' machine type several
Re: [libvirt] [PATCH 3/6] admin: Drop 'internal.h' include from libvirt-admin.h
On 11/08/15 10:22, Martin Kletzander wrote: On Tue, Aug 11, 2015 at 09:58:58AM +0200, Erik Skultety wrote: This is a public library, it shouldn't include anything that is internal. Including the library in it's current state to an example application fails the preprocessor phase. --- include/libvirt/libvirt-admin.h | 2 -- 1 file changed, 2 deletions(-) ACK as-is (independent of other patches, feel free to push this ASAP. pushed now, thanks. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
Hello! I liked the idea of adding all this if user supplies e.g. controller type='pci' model='pci-root'/ IMHO there would be a usability problem with this: a) Too much excessive typing. b) It is not intuitively understood that you have to define the controller explicitly. Especially because if you run qemu by hands, this is not true. For example, when i first started to cope with PCI devices, i quickly found out about address type='pci' in google. But having to add something else is not that obvious and easy to discover. c) AFAIK virt-manager even cannot add PCI controller explicitly, it relies on libvirt's automagic in question. I think it would be a perfect consensus if we add PCI devices are actually present to the condition. In this case, if there is at least one PCI device in the input XML, controller description is automatically added. If there is none, it's OK to remove it. IMHO it's not difficult to implement because it's post-parse stage, and everything that the user entered has already been parsed. But we still want the capability check because old qemu versions simply cannot accommodate this. Therefore it doesn't remove the need to fix up tests. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Remove empty resource partition created by libvirt
On Tue, Aug 11, 2015 at 04:57:15PM +0530, Nikunj A Dadhania wrote: The default resource partition is created in the domain start path if it is not existing. Even when libvirtd is stopped after shutting down all domains, the resource partition still exists. The patch adds code to removes the default resource partition in the cgroup removal path of the domain. If the default resource partition is found to have no child cgroup, the default resource partition will be removed. Moreover, the code does not remove the user provided resource partitions. Signed-off-by: Nikunj A Dadhania nik...@linux.vnet.ibm.com I don't think we want to be doing this. In non-systemd hosts this will be deleting the heirarchy that the sysadmin manually pre-created for their VMs. In a systemd host it will also end up deleting slices that were created by systemd. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Drive hot-unplug: reliable parsing of HMP results
On Mon, Aug 10, 2015 at 05:37:30PM +0200, Frank Schreuder wrote: Hot-unplugging a disk from a guest that supports hot-unplugging generates an error in the libvirt log when running QEMU with the -msg timestamp=on flag. 2015-08-06 10:48:59.945+: 11662: error : qemuMonitorTextDriveDel:2594 : operation failed: deleting drive-virtio-disk4 drive failed: 2015-08-06T10:48:59.945058Z Device 'drive-virtio-disk4' not found This error is caused because the HMP results are getting prefixed with a timestamp. Parsing the output is not reliable with STRPREFIX as the results can be prefixed with a timestamp. Using strstr ensures that parsing the output works whether the results are prefixed or not. Cc: Stefan Hajnoczi stefa...@redhat.com Cc: Daniel P. Berrange berra...@redhat.com Signed-off-by: Frank Schreuder fschreu...@transip.nl --- src/qemu/qemu_monitor_text.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi stefa...@redhat.com pgpIuErhuGmed.pgp Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers
[reducing the cc-list] On Tue, Aug 11, 2015 at 02:47:09AM -0400, Laine Stump wrote: This started out as a fix for a crash reported in IRC and on libvir-list: https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html but as I examined the existing code I found so many small nits to pick in surrounding functions that I decided to fix them all in this patch. The most important fix here is that virNetDevGetFeatures was creating a struct ethtool_gfeatures object as a local on the stack and, although the struct is defined with 0 elements in its features array, we were telling the ethtool ioctl that we had made space for 2 elements. This led to a crash, as outlined in the email above. The fix for this is to allocate the memory for the ethtool_gfeatures object using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type ethtool_get_features_block Beyond that crash fixer, the following fixups were made to the hierarchy of functions between virNetDevGetFeatures() and virNetDevSendEthtoolIoctl(): This looks like an enumeration of things that should've been separate :) * macros used to examine the gfeatures bits were renamed from FEATURE_* to GFEATURE_* virNetDevSentEthtoolIoctl(): * no need to initialize sock to -1, since it is always set at the top of the function. No functional changes here. * remove VIR_DEBUG log (and subsequent *skipping* of error log!) when errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were ever encountered, this would have been *very* problematic, as it would have led to one of those situations where virsh reports an error was encountered but the cause is unknown (or whatever the message is when we have an error but no log message). This does not seem problematic, since we do not treat -1 as a failure. * always call VIR_FORCE_CLOSE(sock) since we know that sock is either a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip). No fucntional change there either. virNetDevFeatureAvailable() * simplify it - no need for ret. * follow libvirt convention of checking for bobLobLaw(lawblog) 0 instead of !bobLobLaw(lawblog). virNetDevGFeatureAvailable() * eliminate this function, as it was ill-conceived (it really was only checking for one gfeature (TX_UDP_TNL), not *any* gfeature. virNetDevGetFeatures() * move all data tables (struct elem) out of the function so that they will be statics instead of taking up space on the stack. * remove pointless/incorrect initialization of i = -1. * remove unnecessary static initialization of struct ethtool_value cmd * g_cmd is now a pointer instead of automatic * use libvirt convention of checking return from virNetDevFeatureAvailable() 0, instead of !virNetDevFeatureAvailable(), and immediately return to caller with an error when virNetDevFeatureAvailable() returns an error (previously it was ignored). This is the change that makes the lack of error reporting problematic. * remove superfluous size_t j, and just re-use i instead. Another style cleanup that could be separated. * runtime allocation/free of proper size object for ethtoolfeatures as described above. * directly call virNetDevSentEthtoolIoctl() instead of now defunct virNetDevGFeatureAvailable(). --- V2: I had been looking for something like VIR_ALLOC_VAR() when writing the patch originally, but somehow missed it, and did an ugly hack with VIR_ALLOC_N and a union. In this version I clean that up and use VIR_ALLOC_VAR() instead. NB: This patch does *not* attempt to determine the proper number of elements for the gfeature array at runtime, as proposed in this patch: https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html since that wasn't the cause of the crash. I'll leave it up to Moshe to repost that patch rebased around this one (or whatever ends up being pushed) if he thinks there is value to it. Also - as I mentioned in earlier mail in response to the crash, I noticed when looking into the gfeatures ethtool code that it looks to me like TX_UDP_TNL should actually be 26 rather than 25, but I may be missing something. Moshe - can you either confirm or deny that? Where did you get the value 25 from? src/util/virnetdev.c | 177 +++ 1 file changed, 79 insertions(+), 98 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1e20789..05fbff5 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -91,10 +91,10 @@ VIR_LOG_INIT(util.netdev); #if HAVE_DECL_ETHTOOL_GFEATURES # define TX_UDP_TNL 25 # define GFEATURES_SIZE 2 Do we need to (re)define these? I'd expect these constants to be present in some header file. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/3] qemu: Keep numad hint after reboot
The numad hint stored in priv-autoNodeset is information that gets lost
during daemon restart. And because we would like to use that
information in the future, we also need to save it in the status XML.
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/qemu/qemu_domain.c | 32 +++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d4b4a28d0d0b..9666116aa958 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -618,21 +618,33 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
virBufferAddLit(buf, /devices\n);
}
+if (priv-autoNodeset) {
+char *nodeset = virBitmapFormat(priv-autoNodeset);
+
+if (!nodeset)
+return -1;
+
+virBufferAsprintf(buf, numad nodeset='%s'/\n, nodeset);
+VIR_FREE(nodeset);
+}
+
return 0;
}
static int
qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
virDomainObjPtr vm,
- virDomainDefParserConfigPtr config
ATTRIBUTE_UNUSED)
+ virDomainDefParserConfigPtr config)
{
qemuDomainObjPrivatePtr priv = vm-privateData;
+virQEMUDriverPtr driver = config-priv;
char *monitorpath;
char *tmp;
int n;
size_t i;
xmlNodePtr *nodes = NULL;
virQEMUCapsPtr qemuCaps = NULL;
+virCapsPtr caps = NULL;
if (VIR_ALLOC(priv-monConfig) 0)
goto error;
@@ -804,15 +816,33 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
}
VIR_FREE(nodes);
+if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
+goto error;
+
+if ((tmp = virXPathString(string(./numad/@nodeset), ctxt))) {
+if (virBitmapParse(tmp, 0, priv-autoNodeset,
+ caps-host.nnumaCell) 0)
+goto error;
+
+if (!(priv-autoCpuset = virCapabilitiesGetCpusForNodemask(caps,
+
priv-autoNodeset)))
+goto error;
+}
+virObjectUnref(caps);
+caps = NULL;
+VIR_FREE(tmp);
+
return 0;
error:
virDomainChrSourceDefFree(priv-monConfig);
priv-monConfig = NULL;
VIR_FREE(nodes);
+VIR_FREE(tmp);
virStringFreeList(priv-qemuDevices);
priv-qemuDevices = NULL;
virObjectUnref(qemuCaps);
+virObjectUnref(caps);
return -1;
}
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/20] tests: Temporarily disable ppc64 cpu tests
On Mon, Aug 10, 2015 at 10:55:53 +0200, Andrea Bolognani wrote: The upcoming commits will make heavy modifications to the ppc64 driver, split so that it's easier to review the changes. Instead of updating the test cases so that they pass, possibly only to update them again with the following commit, disable them for the time being. Another commit will update them all in one go once all required changes are in place. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.
On 08/11/2015 04:14 AM, Simon Kelley wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 10/08/15 22:29, Laine Stump wrote: On 08/10/2015 01:08 PM, Maxim Perevedentsev wrote: This is a fix for commit db488c79173b240459c7754f38c3c6af9b432970 dnsmasq main process exits without waiting for DAD, this is dnsmasq daemon's task. So we periodically poll the kernel using netlink and check whether there are any IPv6 addresses assigned to bridge which have 'tentative' state. After DAD is finished, execution continues. I guess that is what dnsmasq was assumed to do. Since the comments in our code imply that dnsmasq should be waiting for DAD to complete prior to daemonizing, before pushing a fix like this I'd like to find out from the dnsmasq folks if we are erroneously relying on nonexistent dnsmasq behavior, or if maybe there is a bug in some version of dnsmasq. Simon (or other dnsmasq people) - when dnsmasq is run with enable-ra, does it make sure it completes DAD prior to daemonizing? Or does libvirt need to do this extra polling to assure that DAD has completed? (or maybe there's some other config parameter we need to add?) Dnsmasq doesn't wait for DAD to complete before returning. Internally, it know is DAD is still happening on an interface, as it needs to delay calling bind() until after it's complete. It would, therefore be relatively simple to add this behaviour, but it's not a complete solution, since new interfaces can appear _after_ dnsmasq has completed start-up. Having libvirt do its own checks whenever it creates an interface might therefore be a cleaner way of doing things, but I don't have an objection to changing dnsmasq behaviour if there's a good reason why that's not sensible. No need for dnsmasq to change its behavior. I just wanted to make sure that it was the comment in libvirt that was wrong, and not a regression in dnsmasq. Based on your answer, it appears that this was a misunderstanding by the original author of the libvirt code, so it is something that libvirt needs to fix. Thanks for the quick response! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: Don't try formating non-existing addresses
Commit a6f9af8292b6 added checking for address colisions between
starting and ending addresses of forwarding addresses, but forgot that
there might be no addresses set at all.
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/conf/network_conf.c| 22 +++---
xml = nat-network-forward-nat-no-address.xml} | 1 -
xml = nat-network-forward-nat-no-address.xml} | 1 -
tests/networkxml2xmltest.c | 1 +
4 files changed, 20 insertions(+), 5 deletions(-)
copy tests/networkxml2xmlin/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
copy tests/networkxml2xmlout/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 374d723788e1..f9d894b12046 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1731,9 +1731,25 @@ virNetworkForwardNatDefParseXML(const char *networkName,
goto cleanup;
}
-/* verify that start = end */
-if (virSocketAddrGetRange(def-addr.start, def-addr.end, NULL, 0) 0)
-goto cleanup;
+if (addrStart addrEnd) {
+/* verify that start = end */
+if (virSocketAddrGetRange(def-addr.start, def-addr.end, NULL, 0)
0)
+goto cleanup;
+} else {
+if (addrStart) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only start address '%s' specified in nat in
+ forward in network '%s'),
+ addrStart, networkName);
+goto cleanup;
+}
+if (addrEnd) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only end address '%s' specified in nat in
+ forward in network '%s'),
+ addrEnd, networkName);
+}
+}
/* ports for SNAT and MASQUERADE */
nNatPorts = virXPathNodeSet(./port, ctxt, natPortNodes);
diff --git a/tests/networkxml2xmlin/nat-network-forward-nat-address.xml
b/tests/networkxml2xmlin/nat-network-forward-nat-no-address.xml
similarity index 93%
copy from tests/networkxml2xmlin/nat-network-forward-nat-address.xml
copy to tests/networkxml2xmlin/nat-network-forward-nat-no-address.xml
index 403d058330ea..97a64526e94f 100644
--- a/tests/networkxml2xmlin/nat-network-forward-nat-address.xml
+++ b/tests/networkxml2xmlin/nat-network-forward-nat-no-address.xml
@@ -4,7 +4,6 @@
bridge name=virbr0/
forward mode=nat dev=eth1
nat
- address start='10.20.30.40' end='10.20.30.44'/
port start='6' end='65432'/
/nat
/forward
diff --git a/tests/networkxml2xmlout/nat-network-forward-nat-address.xml
b/tests/networkxml2xmlout/nat-network-forward-nat-no-address.xml
similarity index 93%
copy from tests/networkxml2xmlout/nat-network-forward-nat-address.xml
copy to tests/networkxml2xmlout/nat-network-forward-nat-no-address.xml
index faeba2470661..f19e34deecec 100644
--- a/tests/networkxml2xmlout/nat-network-forward-nat-address.xml
+++ b/tests/networkxml2xmlout/nat-network-forward-nat-no-address.xml
@@ -3,7 +3,6 @@
uuid81ff0d90-c91e-6742-64da-4a736edb9a9b/uuid
forward dev='eth1' mode='nat'
nat
- address start='10.20.30.40' end='10.20.30.44'/
port start='6' end='65432'/
/nat
interface dev='eth1'/
diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
index 290336edbefd..8d60aa8fc665 100644
--- a/tests/networkxml2xmltest.c
+++ b/tests/networkxml2xmltest.c
@@ -99,6 +99,7 @@ mymain(void)
DO_TEST(nat-network-dns-forward-plain);
DO_TEST(nat-network-dns-forwarders);
DO_TEST(nat-network-forward-nat-address);
+DO_TEST(nat-network-forward-nat-no-address);
DO_TEST(8021Qbh-net);
DO_TEST(direct-net);
DO_TEST(host-bridge-net);
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: validate network NAT range
On 08/11/2015 08:12 AM, Martin Kletzander wrote: Me and Michal are looking at it, but after this patch I miss the default network after the daemon is restarted. Reverting fixes it, I know nothing more right now. Will update you later on. Bah. I see the problem - if there is a nat element but no address subelement, I am still doing the range validation (and thus validating two empty addresses). Patch coming up. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virNetSocketCheckProtocols: handle EAI_NONAME as IPv6 unavailable
When running the test suite using unshare -n we might have IPv6 but no
configured addresses. Due to AI_ADDRCONFIG getaddrinfo then fails with
EAI_NONAME which we should then treat as IPv6 unavailable.
---
src/rpc/virnetsocket.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 106d09a..5e5f1ab 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -183,7 +183,8 @@ int virNetSocketCheckProtocols(bool *hasIPv4,
if ((gaierr = getaddrinfo(::1, NULL, hints, ai)) != 0) {
if (gaierr == EAI_ADDRFAMILY ||
-gaierr == EAI_FAMILY) {
+gaierr == EAI_FAMILY ||
+gaierr == EAI_NONAME) {
*hasIPv6 = false;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 2/4] qemu: Add PCI-Express root to ARM virt machine
On Tue, Aug 11, 2015 at 09:28:21AM +0100, Daniel P. Berrange wrote:
On Mon, Aug 10, 2015 at 12:08:02PM -0400, Laine Stump wrote:
On 08/10/2015 11:09 AM, Daniel P. Berrange wrote:
On Thu, Aug 06, 2015 at 07:46:58PM +0200, Martin Kletzander wrote:
On Thu, Aug 06, 2015 at 06:37:41PM +0200, Martin Kletzander wrote:
On Fri, Jul 17, 2015 at 02:27:45PM +0300, Pavel Fedin wrote:
Here we assume that if qemu supports generic PCI host controller,
it is a part of virt machine and can be used for adding PCI devices.
In qemu this is actually a PCIe bus, so we also declare multibus
capability so that 0'th bus is specified to qemu correctly as 'pcie.0'
Signed-off-by: Pavel Fedin p.fe...@samsung.com
---
src/qemu/qemu_capabilities.c | 8
src/qemu/qemu_domain.c | 17 +
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d570fdd..f3486c7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2138,6 +2138,14 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr
qemuCaps,
return false;
}
+if (ARCH_IS_ARM(def-os.arch)) {
+/* If 'virt' supports PCI, it supports multibus.
+ * No extra conditions here for simplicity.
+ */
So every ARM qemu with the virt machine type supports both PCI and
multiqueue? How about those virt-* for which you check below. That
might not be related, I'm just curious.
+if (STREQ(def-os.machine, virt))
+return true;
+}
+
return false;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8b050a0..c7d14e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -981,7 +981,7 @@ virDomainXMLNamespace virQEMUDriverDomainXMLNamespace
= {
static int
qemuDomainDefPostParse(virDomainDefPtr def,
virCapsPtr caps,
- void *opaque ATTRIBUTE_UNUSED)
+ void *opaque)
{
bool addDefaultUSB = true;
bool addImplicitSATA = false;
@@ -1030,12 +1030,21 @@ qemuDomainDefPostParse(virDomainDefPtr def,
break;
case VIR_ARCH_ARMV7L:
- addDefaultUSB = false;
- addDefaultMemballoon = false;
- break;
case VIR_ARCH_AARCH64:
addDefaultUSB = false;
addDefaultMemballoon = false;
+ if (STREQ(def-os.machine, virt) ||
+ STRPREFIX(def-os.machine, virt-)) {
+ virQEMUDriverPtr driver = opaque;
+
+ /* This condition is actually a (temporary) hack for test
suite which
+* does not create capabilities cache */
Few questions here. a) how temporary is this since you're not
removing it in this series? b) for what tests you need this hack and
what part of the below is the hack?
Moreover, you cannot use capabilities when defining an XML. The
emulator can change between the domain is defined and started, so you
cannot know with what emulator this will be started.
I see Michal (Cc'd) just pushed this, I probably just missed the mail
Of course I forgot, Cc'ing now.
I agree with your core statement that we should not be using the QEMU
capabilities when defining the XML. With all existing scenarios we have
been able to determine whether to add the implicit PCI controller based
on the machine type name only, because with every other QEMU arch when
doing such a major change as adding a PCI bus, they have created a new
machine type. The problem is that arm 'virt' machine type is not stable,
it is being changed arbitrarily in new QEMU releases :-(
So AFAIK, that leaves us with 3 choices
- Never add PCI controller at time the XML is defined, on the basis
that we have to be conservative in what we add to cope with old QEMU
- Always add PCI controller at time the XML is defined, on the basis
that most people will have new enough QEMU because ARM 'virt' machine
type is very much still in development, so no one will serously stick
with the older QEMU versions which lack PCI.
- Use the capabilities in XML post-parse to conditionally add the
PCI controller. This is what was currently merged
I don't think option 1 makes much sense as it'll harm ARM arch forever
more, to cope with QEMU versions that will almost never be used in
practice.
I'd be inclined to go with option 2, and then if any PCI devices are
actually used with the guest, check the capability at start time when
we are doing auto-address assignment.
Except that auto-address assignment happens after the parse, but before
we write the config to disk (i.e. we don't wait until the domain is
started. I'm guessing that was originally done because we didn't want to
be potentially re-writing the config to disk every time the domain was
started.) Also, we can't assume that no PCI devices used at start
means no PCI devices will ever be desired because hotplug.
Ok, so it sounds to me like we are pretty much forced in to a
[libvirt] [PATCH v2 1/3] conf: Pass private data to Parse function of XML options
This needs a reorder of XML option definitions. It might come in handy
one day.
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 17 +
src/libxl/libxl_domain.c | 3 ++-
src/lxc/lxc_domain.c | 3 ++-
src/qemu/qemu_domain.c | 3 ++-
5 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fd0450f09b37..8199ac0fe16e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16514,7 +16514,7 @@ virDomainObjParseXML(xmlDocPtr xml,
VIR_FREE(nodes);
if (xmlopt-privateData.parse
-xmlopt-privateData.parse(ctxt, obj) 0)
+xmlopt-privateData.parse(ctxt, obj, xmlopt-config) 0)
goto error;
return obj;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9762c4f27698..daa475dfcc71 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2391,14 +2391,6 @@ typedef bool (*virDomainObjListACLFilter)(virConnectPtr
conn,
typedef struct _virDomainXMLOption virDomainXMLOption;
typedef virDomainXMLOption *virDomainXMLOptionPtr;
-typedef void *(*virDomainXMLPrivateDataAllocFunc)(void);
-typedef void (*virDomainXMLPrivateDataFreeFunc)(void *);
-typedef virObjectPtr (*virDomainXMLPrivateDataNewFunc)(void);
-typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr,
- virDomainObjPtr);
-typedef int (*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr,
-virDomainObjPtr);
-
/* Called once after everything else has been parsed, for adjusting
* overall domain defaults. */
typedef int (*virDomainDefPostParseCallback)(virDomainDefPtr def,
@@ -2427,6 +2419,15 @@ struct _virDomainDefParserConfig {
unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN];
};
+typedef void *(*virDomainXMLPrivateDataAllocFunc)(void);
+typedef void (*virDomainXMLPrivateDataFreeFunc)(void *);
+typedef virObjectPtr (*virDomainXMLPrivateDataNewFunc)(void);
+typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr,
+ virDomainObjPtr);
+typedef int (*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr,
+virDomainObjPtr,
+virDomainDefParserConfigPtr);
+
typedef struct _virDomainXMLPrivateDataCallbacks
virDomainXMLPrivateDataCallbacks;
typedef virDomainXMLPrivateDataCallbacks *virDomainXMLPrivateDataCallbacksPtr;
struct _virDomainXMLPrivateDataCallbacks {
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 224ff773f938..40dcea171c06 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -224,7 +224,8 @@ libxlDomainObjPrivateFree(void *data)
static int
libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
- virDomainObjPtr vm)
+ virDomainObjPtr vm,
+ virDomainDefParserConfigPtr config
ATTRIBUTE_UNUSED)
{
libxlDomainObjPrivatePtr priv = vm-privateData;
diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
index 70606f376583..2f377d8dcba3 100644
--- a/src/lxc/lxc_domain.c
+++ b/src/lxc/lxc_domain.c
@@ -65,7 +65,8 @@ virLXCDomainObjPrivateXMLFormat(virBufferPtr buf,
static int
virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
- virDomainObjPtr vm)
+ virDomainObjPtr vm,
+ virDomainDefParserConfigPtr config
ATTRIBUTE_UNUSED)
{
virLXCDomainObjPrivatePtr priv = vm-privateData;
unsigned long long thepid;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ad9a8ac1aaec..d4b4a28d0d0b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -623,7 +623,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
static int
qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
- virDomainObjPtr vm)
+ virDomainObjPtr vm,
+ virDomainDefParserConfigPtr config
ATTRIBUTE_UNUSED)
{
qemuDomainObjPrivatePtr priv = vm-privateData;
char *monitorpath;
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 14/20] cpu: Simplify ppc64 part of CPU map XML
On Mon, Aug 10, 2015 at 10:55:56 +0200, Andrea Bolognani wrote: Use multiple PVRs per CPU model to reduce the number of models we need to keep track of. Remove specific CPU models (eg. POWER7+_v2.1): the corresponding generic CPU model (eg. POWER7) should be used instead to ensure the guest can be booted on any compatible host. Get rid of all the entries that did not match any of the CPU models supported by QEMU, like power8 and power8e. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1250977 ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] virsh: Implement VIR_DOMAIN_BANDWIDTH_IN_FLOOR
On 08/11/2015 06:08 AM, Michal Privoznik wrote: On 11.08.2015 03:25, John Ferlan wrote: On 08/03/2015 02:39 AM, Michal Privoznik wrote: We have a function parseRateStr() that parses --inbound and --outbound arguments to both attach-interface and domiftune. Now that we have all virTypedParams macros needed for QoS, lets parse even floor attribute. The extended format for the arguments looks like this then: --inbound average[,peak[,burst[,floor]]] Signed-off-by: Michal Privoznik mpriv...@redhat.com --- tools/virsh-domain.c | 23 ++- tools/virsh.pod | 12 ++-- 2 files changed, 24 insertions(+), 11 deletions(-) ACK with the squashed in code - now it makes more sense too ;-) (both because of the extra comments/code and some sleep) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 18/20] cpu: Forbid model fallback in the ppc64 driver
On Mon, Aug 10, 2015 at 10:56:00 +0200, Andrea Bolognani wrote: Unlike what happens on x86, on ppc64 you can't mix and match CPU features to obtain the guest CPU you want regardless of the host CPU, so the concept of model fallback doesn't apply. Make sure CPU definitions emitted by the driver, eg. as output of the cpuBaseline() and cpuUpdate() calls, reflect this fact. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virNetSocketCheckProtocols: handle EAI_NONAME as IPv6 unavailable
On Tue, Aug 11, 2015 at 01:46:44PM +0200, Guido Günther wrote:
When running the test suite using unshare -n we might have IPv6 but no
configured addresses. Due to AI_ADDRCONFIG getaddrinfo then fails with
EAI_NONAME which we should then treat as IPv6 unavailable.
---
src/rpc/virnetsocket.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 106d09a..5e5f1ab 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -183,7 +183,8 @@ int virNetSocketCheckProtocols(bool *hasIPv4,
if ((gaierr = getaddrinfo(::1, NULL, hints, ai)) != 0) {
if (gaierr == EAI_ADDRFAMILY ||
-gaierr == EAI_FAMILY) {
+gaierr == EAI_FAMILY ||
+gaierr == EAI_NONAME) {
*hasIPv6 = false;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
ACK
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 20/20] tests: Add a bunch of cpu test case for ppc64
On Mon, Aug 10, 2015 at 10:56:02 +0200, Andrea Bolognani wrote: The test cases cover the cpuCompare(), cpuBaseline() and cpuNodeData() implementation. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/3] qemu: Use numad information when getting pin information
Pinning information returned for emulatorpin and vcpupin calls is being
returned from our data without querying cgroups for some time. However,
not all the data were utilized. When automatic placement is used the
information is not returned for the calls mentioned above. Since the
numad hint in private data is properly saved/restored, we can safely use
it to return true information.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1162947
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/qemu/qemu_driver.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6998e1205a47..7bc0e38b6ebe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5228,6 +5228,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
int ret = -1;
int hostcpus, vcpu;
virBitmapPtr allcpumap = NULL;
+qemuDomainObjPrivatePtr priv = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -5248,6 +5249,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
goto cleanup;
virBitmapSetAll(allcpumap);
+priv = vm-privateData;
/* Clamp to actual number of vcpus */
if (ncpumaps def-vcpus)
@@ -5266,6 +5268,9 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
if (pininfo pininfo-cpumask)
bitmap = pininfo-cpumask;
+else if (vm-def-placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO
+ priv-autoCpuset)
+bitmap = priv-autoCpuset;
else
bitmap = allcpumap;
@@ -5416,6 +5421,7 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
int hostcpus;
virBitmapPtr cpumask = NULL;
virBitmapPtr bitmap = NULL;
+qemuDomainObjPrivatePtr priv = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -5432,10 +5438,15 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
if ((hostcpus = nodeGetCPUCount(NULL)) 0)
goto cleanup;
+priv = vm-privateData;
+
if (def-cputune.emulatorpin) {
cpumask = def-cputune.emulatorpin;
} else if (def-cpumask) {
cpumask = def-cpumask;
+} else if (vm-def-placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO
+ priv-autoCpuset) {
+cpumask = priv-autoCpuset;
} else {
if (!(bitmap = virBitmapNew(hostcpus)))
goto cleanup;
--
2.5.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/3] qemu: Return true pinning info
First two patches just prepare the ground for the third one that explains what needs to be fixed and ho it's done. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1162947 v2: - Added more info to commit message of patch 2/3 - Changed the commit subject of patch 3/3 Martin Kletzander (3): conf: Pass private data to Parse function of XML options qemu: Keep numad hint after reboot qemu: Use numad information when getting pin information src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 17 + src/libxl/libxl_domain.c | 3 ++- src/lxc/lxc_domain.c | 3 ++- src/qemu/qemu_domain.c | 33 - src/qemu/qemu_driver.c | 11 +++ 6 files changed, 57 insertions(+), 12 deletions(-) -- 2.5.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 17/20] cpu: Implement backwards compatibility in the ppc64 driver
On Mon, Aug 10, 2015 at 10:55:59 +0200, Andrea Bolognani wrote:
All previously recognized CPU models (POWER7_v2.1, POWER7_v2.3,
POWER7+_v2.1 and POWER8_v1.0) are internally converted to the
corrisponding generation name so that existing guests don't stop
working.
---
src/cpu/cpu_ppc64.c | 36 ++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index bf8f4da..d87e0d1 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -57,6 +57,33 @@ struct ppc64_map {
struct ppc64_model *models;
};
+/* Convert a legacy CPU definition by transforming
+ * model names to generation names:
+ * POWER7_v2.1 = POWER7
+ * POWER7_v2.3 = POWER7
+ * POWER7+_v2.1 = POWER7
+ * POWER8_v1.0 = POWER8 */
+static virCPUDefPtr
+ppc64LegacyConvertCPUDef(const virCPUDef *legacy)
I think ppc64ConvertLegacyCPUDef would be a slightly better name.
+{
+virCPUDefPtr cpu;
+
+if (!(cpu = virCPUDefCopy(legacy)))
+goto out;
+
+if (!(STREQ(cpu-model, POWER7_v2.1) ||
+ STREQ(cpu-model, POWER7_v2.3) ||
+ STREQ(cpu-model, POWER7+_v2.1) ||
+ STREQ(cpu-model, POWER8_v1.0))) {
+goto out;
+}
+
+cpu-model[strlen(POWERx)] = 0;
+
+ out:
+return cpu;
+}
+
static void
ppc64DataFree(virCPUppc64Data *data)
{
@@ -426,18 +453,22 @@ ppc64MakeCPUData(virArch arch,
static virCPUCompareResult
ppc64Compute(virCPUDefPtr host,
- const virCPUDef *cpu,
+ const virCPUDef *other,
virCPUDataPtr *guestData,
char **message)
{
struct ppc64_map *map = NULL;
struct ppc64_model *host_model = NULL;
struct ppc64_model *guest_model = NULL;
-
+virCPUDefPtr cpu = NULL;
virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;
virArch arch;
size_t i;
+/* Ensure existing configurations are handled correctly */
+if (!(cpu = ppc64LegacyConvertCPUDef(other)))
+goto cleanup;
+
if (cpu-arch != VIR_ARCH_NONE) {
bool found = false;
When reading the tests, I realized we don't do any compatibility hacks
for *Baseline, which is correct because it never worked on Power, but we
should explicitly document it inside ppc64Baseline to avoid confusion in
the future.
@@ -506,6 +537,7 @@ ppc64Compute(virCPUDefPtr host,
ret = VIR_CPU_COMPARE_IDENTICAL;
cleanup:
+virCPUDefFree(cpu);
ppc64MapFree(map);
ppc64ModelFree(host_model);
ppc64ModelFree(guest_model);
ACK with the comment added.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] util: Remove empty resource partition created by libvirt
The default resource partition is created in the domain start path if it
is not existing. Even when libvirtd is stopped after shutting down all
domains, the resource partition still exists.
The patch adds code to removes the default resource partition in the
cgroup removal path of the domain. If the default resource partition is
found to have no child cgroup, the default resource partition will be
removed.
Moreover, the code does not remove the user provided resource
partitions.
Signed-off-by: Nikunj A Dadhania nik...@linux.vnet.ibm.com
---
src/util/vircgroup.c | 55
1 file changed, 55 insertions(+)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 0599ba5..4dc0702 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -42,6 +42,7 @@
#define __VIR_CGROUP_ALLOW_INCLUDE_PRIV_H__
#include vircgrouppriv.h
+#include dirname.h
#include virutil.h
#include viralloc.h
#include virerror.h
@@ -3311,6 +3312,59 @@ virCgroupRemoveRecursively(char *grppath)
/**
+ * virCgroupRemoveEmptyParent:
+ *
+ * @group-path: The group path
+ *
+ * Cleanup the libvirt created partition directory if there are no
+ * child group existing. For the given @group-path, find the parent
+ * directory. For resource partition created by libvirt check
+ * existence of child cgroup. Remove the resource partition if no
+ * child cgroup exist.
+ *
+ * Returns: void
+ */
+static void
+virCgroupRemoveEmptyParent(char *grppath)
+{
+char *parent = NULL, *partition = NULL;
+struct dirent *ent;
+DIR *grpdir;
+int direrr;
+int is_empty = 1;
+
+if (!(parent = mdir_name(grppath)))
+goto cleanup;
+
+partition = strrchr(parent, '/');
+if (STRNEQ(partition, /machine) STRNEQ(partition, /machine.slice))
+goto cleanup;
+
+grpdir = opendir(parent);
+if (grpdir == NULL)
+goto cleanup;
+
+while ((direrr = virDirRead(grpdir, ent, NULL)) 0) {
+if (ent-d_name[0] == '.') continue;
+if (ent-d_type == DT_DIR) {
+is_empty = 0;
+break;
+}
+}
+closedir(grpdir);
+
+if (is_empty) {
+VIR_DEBUG(Removing empty parent cgroup %s, parent);
+if (rmdir(parent) != 0)
+VIR_ERROR(_(Unable to remove %s (%d)), parent, errno);
+}
+
+ cleanup:
+VIR_FREE(parent);
+return;
+}
+
+/**
* virCgroupRemove:
*
* @group: The group to be removed
@@ -3352,6 +3406,7 @@ virCgroupRemove(virCgroupPtr group)
VIR_DEBUG(Removing cgroup %s and all child cgroups, grppath);
rc = virCgroupRemoveRecursively(grppath);
+virCgroupRemoveEmptyParent(grppath);
VIR_FREE(grppath);
}
VIR_DEBUG(Done removing cgroup %s, group-path);
--
2.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Drive hot-unplug: reliable parsing of HMP results
On Mon, Aug 10, 2015 at 05:01:22PM +0100, Daniel P. Berrange wrote:
On Mon, Aug 10, 2015 at 04:49:03PM +0100, Stefan Hajnoczi wrote:
On Mon, Aug 10, 2015 at 4:37 PM, Frank Schreuder fschreu...@transip.nl
wrote:
Hot-unplugging a disk from a guest that supports hot-unplugging generates
an error
in the libvirt log when running QEMU with the -msg timestamp=on flag.
2015-08-06 10:48:59.945+: 11662: error : qemuMonitorTextDriveDel:2594
:
operation failed: deleting drive-virtio-disk4 drive failed:
2015-08-06T10:48:59.945058Z Device 'drive-virtio-disk4' not found
This error is caused because the HMP results are getting prefixed with a
timestamp.
Parsing the output is not reliable with STRPREFIX as the results can be
prefixed with a timestamp.
Using strstr ensures that parsing the output works whether the results
are prefixed or not.
Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Daniel P. Berrange berra...@redhat.com
Signed-off-by: Frank Schreuder fschreu...@transip.nl
---
src/qemu/qemu_monitor_text.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 2aa0460..d5ef089 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -2586,7 +2586,7 @@ int qemuMonitorTextDriveDel(qemuMonitorPtr mon,
/* (qemu) drive_del wark
* Device 'wark' not found */
-} else if (STRPREFIX(reply, Device ') (strstr(reply, not
found))) {
+} else if (strstr(reply, Device ') strstr(reply, not found)) {
/* NB: device not found errors mean the drive was auto-deleted
and we
* ignore the error */
} else if (STRNEQ(reply, )) {
I'm not very familiar with the libvirt codebase, but perhaps the
timestamps on error messages should be stripped in the QEMU monitor
processIO handler functions?
That way any remaining bugs like this will be resolved too.
That would be right froma strict correctness POV, but looking at the
existing monitor handling code it all uses strstr too, so this is
matching existing practice. The text monitor code is scary enough
that it is probably wise not to try changing it at this stage in
its life.
Fair enough.
Stefan
pgpcT2aJvnawK.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: validate network NAT range
On Mon, Aug 10, 2015 at 02:02:50PM +0200, Ján Tomko wrote: On Sat, Aug 08, 2015 at 06:04:00PM -0400, Laine Stump wrote: This patch modifies virSocketAddrGetRange() to function properly when the containing network/prefix of the address range isn't known, for example in the case of the NAT range of a virtual network (since it is a range of addresses on the *host*, not within the network itself). We then take advantage of this new functionality to validate the NAT range of a virtual network. Extra test cases are also added to verify that virSocketAddrGetRange() works properly in both positive and negative cases when the network pointer is NULL. This is the *real* fix for: https://bugzilla.redhat.com/show_bug.cgi?id=985653 Commits 1e334a and 48e8b9 had earlier been pushed as fixes for that bug, but I had neglected to read the report carefully, so instead of fixing validation for the NAT range, I had fixed validation for the DHCP range. sigh. --- The changes to virSocketAddrGetRange() *look* like they are extensive, but really they almost completely consist of: 1) reordering and reindenting some of the checks so that they are only executed when we have a valid network address 2) modifying the error messages that could occur when there isn't a valid network so that they don't attempt to use the network address or prefix. src/conf/network_conf.c | 4 ++ src/util/virsocketaddr.c | 168 +-- tests/sockettest.c | 46 - 3 files changed, 136 insertions(+), 82 deletions(-) Me and Michal are looking at it, but after this patch I miss the default network after the daemon is restarted. Reverting fixes it, I know nothing more right now. Will update you later on. Martin signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 19/20] tests: Re-enable ppc64 cpu tests
On Mon, Aug 10, 2015 at 10:56:01 +0200, Andrea Bolognani wrote: Now that all the changes have been implemented we can run the test cases once again, after updating them to reflect the new behaviour. ACK Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 13/20] cpu: Support multiple PVRs in the ppc64 driver
On Tue, 2015-08-11 at 10:26 +0200, Jiri Denemark wrote:
+if (!virXPathBoolean(boolean(./@value), ctxt) ||
+virXPathULongHex(string(./@value), ctxt, pvr) 0)
{
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _(Missing or invalid PVR value in
CPU model %s),
+ model-name);
+goto ignore;
Wrong indentation, s/^// in the 4 lines above. ACK once it's
fixed.
Good catch.
I've fixed the indentation and removed the unnecessary call
to virXPathBoolean(), as agreed, both here and in patch 15/20.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Don't try formating non-existing addresses
On 08/11/2015 09:21 AM, Martin Kletzander wrote:
On Tue, Aug 11, 2015 at 03:16:16PM +0200, Martin Kletzander wrote:
Commit a6f9af8292b6 added checking for address colisions between
starting and ending addresses of forwarding addresses, but forgot that
there might be no addresses set at all.
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/conf/network_conf.c| 22
+++---
xml = nat-network-forward-nat-no-address.xml} | 1 -
xml = nat-network-forward-nat-no-address.xml} | 1 -
tests/networkxml2xmltest.c | 1 +
4 files changed, 20 insertions(+), 5 deletions(-)
copy tests/networkxml2xmlin/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
copy tests/networkxml2xmlout/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 374d723788e1..f9d894b12046 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1731,9 +1731,25 @@ virNetworkForwardNatDefParseXML(const char
*networkName,
goto cleanup;
}
-/* verify that start = end */
-if (virSocketAddrGetRange(def-addr.start, def-addr.end,
NULL, 0) 0)
-goto cleanup;
+if (addrStart addrEnd) {
+/* verify that start = end */
+if (virSocketAddrGetRange(def-addr.start, def-addr.end,
NULL, 0) 0)
+goto cleanup;
+} else {
+if (addrStart) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only start address '%s' specified in
nat in
+ forward in network '%s'),
+ addrStart, networkName);
+goto cleanup;
+}
+if (addrEnd) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only end address '%s' specified in
nat in
+ forward in network '%s'),
+ addrEnd, networkName);
I see the missing 'goto cleanup;' now, consider this squashed in as it
already is in my tree:
diff --git c/src/conf/network_conf.c w/src/conf/network_conf.c
index f9d894b12046..c1cbd76c6807 100644
--- c/src/conf/network_conf.c
+++ w/src/conf/network_conf.c
@@ -1748,6 +1748,7 @@ virNetworkForwardNatDefParseXML(const char
*networkName,
_(Only end address '%s' specified in nat
in
forward in network '%s'),
addrEnd, networkName);
+goto cleanup;
}
}
ACK to the patch with that squashed in. Thanks for cleaning up after me :-)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] virnetsockettest test binary failed while building libvirt
On Tue, Aug 11, 2015 at 06:38:32PM +0600, Dmitriy Slachshyov wrote: Hi, While building the libvirt version 1.2.18 on CentOS7 and also Fedora 22 with the latest updates I have got an error. I also tried to build version 1.2.17 and had the same result. Please, help to resolve this problem Regurds, Dmitriy == libvirt 1.2.18: tests/test-suite.log == # TOTAL: 132 # PASS: 129 # SKIP: 2 # XFAIL: 0 # FAIL: 1 # XPASS: 0 # ERROR: 0 .. contents:: :depth: 2 FAIL: virnetsockettest == TEST: virnetsockettest 1) Socket TCP/IPv4 Accept... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 2) Socket TCP/IPv6 Accept... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 3) Socket TCP/IPv4+IPv6 Accept ... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 4) Socket TCP/IPv4+IPv6 Accept ... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 5) Socket UNIX Accept... libvirt: XML-RPC error : Cannot write data: Broken pipe OK 6) Socket UNIX Addrs ... OK 7) Socket External Command /dev/zero ... OK 8) Socket External Command /dev/does-not-exist ... libvirt: XML-RPC error : End of file while reading data: /bin/cat: /dev/does-not-exist: No such file or directory: Input/output error OK 9) SSH test 1... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 10) SSH test 2... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 11) SSH test 3... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 12) SSH test 4... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error OK 13) SSH test 5... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 14) SSH test 6... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 15) SSH test 7... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error It looks like you cannot execute 'ssh' because of permissions. Not that I'm an expert on this, I just read the previous line a reworded it... FAILED FAIL virnetsockettest (exit status: 1) SKIP: statstest === TEST: statstest SKIP statstest (exit status: 77) SKIP: reconnect === TEST: reconnect SKIP reconnect (exit status: 77) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 17/20] cpu: Implement backwards compatibility in the ppc64 driver
On Tue, 2015-08-11 at 15:17 +0200, Jiri Denemark wrote: +/* Convert a legacy CPU definition by transforming + * model names to generation names: + * POWER7_v2.1 = POWER7 + * POWER7_v2.3 = POWER7 + * POWER7+_v2.1 = POWER7 + * POWER8_v1.0 = POWER8 */ +static virCPUDefPtr +ppc64LegacyConvertCPUDef(const virCPUDef *legacy) I think ppc64ConvertLegacyCPUDef would be a slightly better name. Agreed, I've changed the name as per your suggestion. When reading the tests, I realized we don't do any compatibility hacks for *Baseline, which is correct because it never worked on Power, but we should explicitly document it inside ppc64Baseline to avoid confusion in the future. I've added a comment explaining the reason why we can't handle that situation any better. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/20] cpu: Fix and improve the ppc64 driver
On Mon, 2015-08-10 at 10:55 +0200, Andrea Bolognani wrote: Patches 01-07 are cleanups, 08-10 are bug fixes and 11-20 improve the driver. Changes in v3: * Fix a bug spotted by John thanks to Coverity Changes in v2: * Implement compatibility with guests defined on older libvirt versions * Always use fallback='forbid' when emitting CPU definitions * Update all tests in one go instead of changing them several times * Handle test failure and system failure differently in cpuTestGuestData() * Simplify ppc64DriverNodeData() * Simplify XML parsing code * Remove unnecessary NULL checks * Add even more test cases * Other small changes such as shuffling chunks between commits to make the history more readable Pushed after addressing your concerns. Thanks for the review :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Don't try formating non-existing addresses
On Tue, Aug 11, 2015 at 03:16:16PM +0200, Martin Kletzander wrote:
Commit a6f9af8292b6 added checking for address colisions between
starting and ending addresses of forwarding addresses, but forgot that
there might be no addresses set at all.
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/conf/network_conf.c| 22 +++---
xml = nat-network-forward-nat-no-address.xml} | 1 -
xml = nat-network-forward-nat-no-address.xml} | 1 -
tests/networkxml2xmltest.c | 1 +
4 files changed, 20 insertions(+), 5 deletions(-)
copy tests/networkxml2xmlin/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
copy tests/networkxml2xmlout/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 374d723788e1..f9d894b12046 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1731,9 +1731,25 @@ virNetworkForwardNatDefParseXML(const char *networkName,
goto cleanup;
}
-/* verify that start = end */
-if (virSocketAddrGetRange(def-addr.start, def-addr.end, NULL, 0) 0)
-goto cleanup;
+if (addrStart addrEnd) {
+/* verify that start = end */
+if (virSocketAddrGetRange(def-addr.start, def-addr.end, NULL, 0)
0)
+goto cleanup;
+} else {
+if (addrStart) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only start address '%s' specified in nat in
+ forward in network '%s'),
+ addrStart, networkName);
+goto cleanup;
+}
+if (addrEnd) {
+virReportError(VIR_ERR_XML_ERROR,
+ _(Only end address '%s' specified in nat in
+ forward in network '%s'),
+ addrEnd, networkName);
I see the missing 'goto cleanup;' now, consider this squashed in as it
already is in my tree:
diff --git c/src/conf/network_conf.c w/src/conf/network_conf.c
index f9d894b12046..c1cbd76c6807 100644
--- c/src/conf/network_conf.c
+++ w/src/conf/network_conf.c
@@ -1748,6 +1748,7 @@ virNetworkForwardNatDefParseXML(const char *networkName,
_(Only end address '%s' specified in nat in
forward in network '%s'),
addrEnd, networkName);
+goto cleanup;
}
}
--
Martin
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Don't try formating non-existing addresses
On 11.08.2015 15:16, Martin Kletzander wrote:
Commit a6f9af8292b6 added checking for address colisions between
starting and ending addresses of forwarding addresses, but forgot that
there might be no addresses set at all.
Signed-off-by: Martin Kletzander mklet...@redhat.com
---
src/conf/network_conf.c| 22
+++---
xml = nat-network-forward-nat-no-address.xml} | 1 -
xml = nat-network-forward-nat-no-address.xml} | 1 -
tests/networkxml2xmltest.c | 1 +
4 files changed, 20 insertions(+), 5 deletions(-)
copy tests/networkxml2xmlin/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
copy tests/networkxml2xmlout/{nat-network-forward-nat-address.xml =
nat-network-forward-nat-no-address.xml} (93%)
ACK with that diff you've posted applied.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] virnetsockettest test binary failed while building libvirt
Hi, While building the libvirt version 1.2.18 on CentOS7 and also Fedora 22 with the latest updates I have got an error. I also tried to build version 1.2.17 and had the same result. Please, help to resolve this problem Regurds, Dmitriy== libvirt 1.2.18: tests/test-suite.log == # TOTAL: 132 # PASS: 129 # SKIP: 2 # XFAIL: 0 # FAIL: 1 # XPASS: 0 # ERROR: 0 .. contents:: :depth: 2 FAIL: virnetsockettest == TEST: virnetsockettest 1) Socket TCP/IPv4 Accept... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 2) Socket TCP/IPv6 Accept... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 3) Socket TCP/IPv4+IPv6 Accept ... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 4) Socket TCP/IPv4+IPv6 Accept ... libvirt: XML-RPC error : End of file while reading data: Input/output error OK 5) Socket UNIX Accept... libvirt: XML-RPC error : Cannot write data: Broken pipe OK 6) Socket UNIX Addrs ... OK 7) Socket External Command /dev/zero ... OK 8) Socket External Command /dev/does-not-exist ... libvirt: XML-RPC error : End of file while reading data: /bin/cat: /dev/does-not-exist: No such file or directory: Input/output error OK 9) SSH test 1... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 10) SSH test 2... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 11) SSH test 3... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 12) SSH test 4... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error OK 13) SSH test 5... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 14) SSH test 6... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED 15) SSH test 7... libvirt: XML-RPC error : End of file while reading data: libvirt: error : cannot execute binary ssh: Permission denied: Input/output error FAILED FAIL virnetsockettest (exit status: 1) SKIP: statstest === TEST: statstest SKIP statstest (exit status: 77) SKIP: reconnect === TEST: reconnect SKIP reconnect (exit status: 77) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] cpu: Fix segfault in the ppc64 driver
Commit adb865d introduced some changes in ppc64DriverNodeData() that cause libvirtd to crash on startup unless this patch is applied as well. --- src/cpu/cpu_ppc64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 33fec8b..85aa5bc 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -628,7 +628,7 @@ ppc64DriverNodeData(virArch arch) if (VIR_ALLOC(nodeData) 0) goto error; -if (VIR_ALLOC(data) 0) +if (VIR_ALLOC(nodeData-data.ppc64) 0) goto error; data = nodeData-data.ppc64; -- 2.4.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu: Fix segfault in the ppc64 driver
On 11.08.2015 17:53, Andrea Bolognani wrote: Commit adb865d introduced some changes in ppc64DriverNodeData() that cause libvirtd to crash on startup unless this patch is applied as well. --- src/cpu/cpu_ppc64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 33fec8b..85aa5bc 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -628,7 +628,7 @@ ppc64DriverNodeData(virArch arch) if (VIR_ALLOC(nodeData) 0) goto error; -if (VIR_ALLOC(data) 0) +if (VIR_ALLOC(nodeData-data.ppc64) 0) goto error; data = nodeData-data.ppc64; ACK Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt master] interface type: add udp socket support
On 8/10/15 11:06 AM, Ján Tomko wrote: Signed-off-by: Jonathan Toppins jtopp...@cumulusnetworks.com --- docs/formatdomain.html.in| 17 src/conf/domain_conf.c | 56 +--- src/conf/domain_conf.h | 3 +++ src/conf/netdev_bandwidth_conf.h | 1 + src/libxl/libxl_conf.c | 1 + src/lxc/lxc_controller.c | 1 + src/lxc/lxc_process.c| 1 + src/qemu/qemu_command.c | 12 + src/qemu/qemu_hotplug.c | 1 + src/qemu/qemu_interface.c| 2 ++ src/uml/uml_conf.c | 5 src/xenconfig/xen_sxpr.c | 1 + tools/virsh-domain.c | 1 + 13 files changed, 99 insertions(+), 3 deletions(-) Missing test cases for tests/qemuxml2argvtest.c and tests/qemuxml2xmltest.c. Didn't know about the unit test area, would be happy to add some unit tests. Is there some text document describing how one runs these tests and needs to set things up or should I just read a good example? diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c0a265a..95f7f5d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4165,6 +4165,23 @@ lt;/devicesgt; .../pre +h5a name=elementsNICSUDPUDP unicast tunnel/a/h5 + +p +A UDP unicast architecture provides a virtual network which enables +connections between Qemu instances using Qemu's UDP infrastructure./p + It would be nice to document what the addresses mean, and mention that this is supported span class=sinceSince 1.2.19/span. ack, do I need to add it after the description paragraph (p/p)? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] util: fix virNetDevSendEthtoolIoctl() and its callers
On 08/11/2015 07:41 AM, Ján Tomko wrote: [reducing the cc-list] On Tue, Aug 11, 2015 at 02:47:09AM -0400, Laine Stump wrote: This started out as a fix for a crash reported in IRC and on libvir-list: https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html but as I examined the existing code I found so many small nits to pick in surrounding functions that I decided to fix them all in this patch. The most important fix here is that virNetDevGetFeatures was creating a struct ethtool_gfeatures object as a local on the stack and, although the struct is defined with 0 elements in its features array, we were telling the ethtool ioctl that we had made space for 2 elements. This led to a crash, as outlined in the email above. The fix for this is to allocate the memory for the ethtool_gfeatures object using VIR_ALLOC_VAR, adding on GFEATURES_SIZE elements of type ethtool_get_features_block Beyond that crash fixer, the following fixups were made to the hierarchy of functions between virNetDevGetFeatures() and virNetDevSendEthtoolIoctl(): This looks like an enumeration of things that should've been separate :) Possibly, but it would increase the bureaucratic overhead by an order of magnitude, and many of these things are too inconsequential to bother changing if they are in a patch by themselves. * macros used to examine the gfeatures bits were renamed from FEATURE_* to GFEATURE_* virNetDevSentEthtoolIoctl(): * no need to initialize sock to -1, since it is always set at the top of the function. No functional changes here. * remove VIR_DEBUG log (and subsequent *skipping* of error log!) when errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were ever encountered, this would have been *very* problematic, as it would have led to one of those situations where virsh reports an error was encountered but the cause is unknown (or whatever the message is when we have an error but no log message). This does not seem problematic, since we do not treat -1 as a failure. Yeah, I hadn't thought of it that way, although this makes it a problem that we are ever logging the errors. I a virReportError logging function is called, we should be failing, and if we're not failing, then we shouldn't be logging an error. I'll rework the stuff to make the lower level function never log errors, and change the high level functions back to ignoring the errors * always call VIR_FORCE_CLOSE(sock) since we know that sock is either a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip). No fucntional change there either. virNetDevFeatureAvailable() * simplify it - no need for ret. * follow libvirt convention of checking for bobLobLaw(lawblog) 0 instead of !bobLobLaw(lawblog). virNetDevGFeatureAvailable() * eliminate this function, as it was ill-conceived (it really was only checking for one gfeature (TX_UDP_TNL), not *any* gfeature. virNetDevGetFeatures() * move all data tables (struct elem) out of the function so that they will be statics instead of taking up space on the stack. * remove pointless/incorrect initialization of i = -1. * remove unnecessary static initialization of struct ethtool_value cmd * g_cmd is now a pointer instead of automatic * use libvirt convention of checking return from virNetDevFeatureAvailable() 0, instead of !virNetDevFeatureAvailable(), and immediately return to caller with an error when virNetDevFeatureAvailable() returns an error (previously it was ignored). This is the change that makes the lack of error reporting problematic. * remove superfluous size_t j, and just re-use i instead. Another style cleanup that could be separated. But where do you draw the line? If each one of these was put in a separate patch, there would be 8 or 10 patches here. But anyway, I cave - I'll separate out the crash fix from all the other refactor stuff and resubmit. * runtime allocation/free of proper size object for ethtoolfeatures as described above. * directly call virNetDevSentEthtoolIoctl() instead of now defunct virNetDevGFeatureAvailable(). --- V2: I had been looking for something like VIR_ALLOC_VAR() when writing the patch originally, but somehow missed it, and did an ugly hack with VIR_ALLOC_N and a union. In this version I clean that up and use VIR_ALLOC_VAR() instead. NB: This patch does *not* attempt to determine the proper number of elements for the gfeature array at runtime, as proposed in this patch: https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html since that wasn't the cause of the crash. I'll leave it up to Moshe to repost that patch rebased around this one (or whatever ends up being pushed) if he thinks there is value to it. Also - as I mentioned in earlier mail in response to the crash, I noticed when looking into the gfeatures ethtool code that it looks to me like TX_UDP_TNL should actually be 26 rather than 25, but I
Re: [libvirt] [PATCH] cpu: Fix segfault in the ppc64 driver
On Tue, 2015-08-11 at 17:59 +0200, Michal Privoznik wrote: On 11.08.2015 17:53, Andrea Bolognani wrote: Commit adb865d introduced some changes in ppc64DriverNodeData() that cause libvirtd to crash on startup unless this patch is applied as well. --- src/cpu/cpu_ppc64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 33fec8b..85aa5bc 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -628,7 +628,7 @@ ppc64DriverNodeData(virArch arch) if (VIR_ALLOC(nodeData) 0) goto error; -if (VIR_ALLOC(data) 0) +if (VIR_ALLOC(nodeData-data.ppc64) 0) goto error; data = nodeData-data.ppc64; ACK Pushed, thanks. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH libvirt master] interface type: add udp socket support
On 8/10/15 1:28 AM, Laine Stump wrote: On 08/07/2015 06:14 PM, Jonathan Toppins wrote: Adds a new interface type using UDP sockets, this seems only applicable to QEMU but have edited tree-wide to support the new interface type. The interface type required the addition of a destaddr (destination address), this then maps into the following xml and qemu call. interface type='udp' mac address='52:54:00:5c:67:56'/ source address='127.0.0.1' port='2'/ model type='virtio'/ dest address=127.0.0.1' port='2'/ I think I don't like the dest element. Historically, source has been used to describe the resources used in the real world to backup the emulated device, or maybe the real world source of the data that's going into the emulated device. What you're doing is kind of redefining it to be the local end of real world resources. Aside from that, for mcast, client, and server interface types, source address is where the remote address for the socket is set. I'd say that to make it fit in better with the existing types, the remote address should be in source address='blah', and the local address should be a subelement of that, something like: source address='remoteaddr' port='remoteport' local address='localaddr' port='localport'/ /source That makes source address exactly fit the same purpose as source address for the existing mcast, server, and client interface types. (It may seem a little odd until you start thinking of source as the thing farthest away from the virtual machine). I am not partial to dest, would be happy to change the tag to something that makes more sense. Would need to think about (or be given some pointers to) how one would process a nested entry like this. Am sure it is simple but didn't really look much into it and followed what the surrounding code had done. From a, I need to generate this by hand or edit it by hand, it would make sense to me to keep the nesting in the interface entry as simple as possible, it seems right now it is only one tag level deep (limited knowledge though). On the other hand nesting it like this would make writing a DTD/XSD easier, not sure if libvirt does this for its xml configurations. Thanks for the comments so far. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2] util: don't overwrite stack when getting ethtool gfeatures
This fixes the crash described here:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
In short, we were calling ioctl(SIOCETHTOOL) pointing to a too-short
object that was a local on the stack, resulting in the memory past the
end of the object being overwritten. This was because the struct used
by the ETHTOOL_GFEATURES command of SIOCETHTOOL ends with a 0-length
array, but we were telling ethtool that it could use 2 elements on the
array.
The fix is to allocate the necessary memory with VIR_ALLOC_VAR(),
including the extra length needed for a 2 element array at the end.
---
V2 difference: eliminate all the other cleanups that were in V1, just
fix the crash. I had misunderstood that a failure of the ioctl
shouldn't really be considered a *failure*, and don't have time to
tweak around with all the inconsequential stuff right now.
src/util/virnetdev.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..c158ca3 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3130,7 +3130,7 @@ virNetDevGetFeatures(const char *ifname,
size_t i = -1;
struct ethtool_value cmd = { 0 };
# if HAVE_DECL_ETHTOOL_GFEATURES
-struct ethtool_gfeatures g_cmd = { 0 };
+struct ethtool_gfeatures *g_cmd;
# endif
struct elem{
const int cmd;
@@ -3188,10 +3188,14 @@ virNetDevGetFeatures(const char *ifname,
# endif
# if HAVE_DECL_ETHTOOL_GFEATURES
-g_cmd.cmd = ETHTOOL_GFEATURES;
-g_cmd.size = GFEATURES_SIZE;
-if (virNetDevGFeatureAvailable(ifname, g_cmd))
+if (VIR_ALLOC_VAR(g_cmd,
+ struct ethtool_get_features_block, GFEATURES_SIZE) 0)
+return -1;
+g_cmd-cmd = ETHTOOL_GFEATURES;
+g_cmd-size = GFEATURES_SIZE;
+if (virNetDevGFeatureAvailable(ifname, g_cmd))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+VIR_FREE(g_cmd);
# endif
if (virNetDevRDMAFeature(ifname, out) 0)
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers
I just posted a much simplified V2 of this patch that *only* fixes the crash, doesn't make any other cleanups: https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 0/4] qemu: Allow PCI virtio on ARM virt machine
On 07/17/2015 07:27 AM, Pavel Fedin wrote: Virt machine in qemu since v2.3.0 has PCI generic host controller, and can use PCI devices. This provides performance improvement as well as vhost-net with irqfd support for virtio-net. However libvirt currently does not allow ARM virt machine to have PCI devices. This patchset adds the necessary support. Changes since v4: - Rebased onto current master - Added possibility to plug virtio-net-pci adapter directly into PCIe bus. This is necessary for irqfds to work in qemu. Changes since v3: - Capability is based not on qemu version but on support of gpex-pcihost device by qemu - Added a workaround, allowing to pass make check. The problem is that test suite does not build capabilities cache. Unfortunately this means that correct unit-test for the new functionality currently cannot be written. Test suite framework needs to be improved. Changes since v2: Complete rework, use different approach - Correctly model PCI Express bus on the machine. It is now possible to explicitly specify address-type='pci' with attributes. This allows to attach not only virtio, but any other PCI device to the model. - Default is not changed and still mmio, for backwards compatibility with existing installations. PCI bus has to be explicitly specified. - Check for the capability in correct place, in v2 it actually did not work Changes since v1: - Added capability based on qemu version number - Recognize also virt- prefix Pavel Fedin (4): qemu: Introduce QEMU_CAPS_OBJECT_GPEX Add PCI-Express root to ARM virt machine Build correct command line for PCI NICs on ARM Allow to plug virtio-net-pci into PCIe slot I was a bit confused about the patches that landed; I see now that they only add a PCI controller for modern -M virt, but don't change the virtio defaults to use it. This is good, and in my brief testing doesn't cause any regressions with current virt-manager. We can figure out later if/how to change the bus=virtio or model=virtio default to use PCI instead of virtio-mmio But yeah, we need to figure out that test case issue. There's already a regression with git head: $ sudo virsh create test-aarch64.xml error: Failed to create domain from test-aarch64.xml error: internal error: autogenerated dmi-to-pci-bridge options not set XML attached. I'm guessing this is one of Laine's patches, CCd - Cole domain type=qemu namerhel7.0-aarch64-2/name uuid94b59510-6a9f-4c08-bf6b-f906642a5473/uuid memory1048576/memory currentMemory1048576/currentMemory vcpu1/vcpu os type arch=aarch64 machine=virthvm/type loader readonly=yes type=pflash/usr/share/edk2.git/aarch64/QEMU_EFI-pflash.raw/loader boot dev=cdrom/ boot dev=hd/ /os cpu mode=custom match=exact modelcortex-a57/model /cpu clock offset=utc/ on_poweroffdestroy/on_poweroff on_rebootdestroy/on_reboot on_crashdestroy/on_crash devices emulator/bin/qemu-system-aarch64/emulator /devices /domain -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2] util: don't overwrite stack when getting ethtool gfeatures
On 08/11/2015 02:05 PM, Laine Stump wrote:
This fixes the crash described here:
https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
In short, we were calling ioctl(SIOCETHTOOL) pointing to a too-short
object that was a local on the stack, resulting in the memory past the
end of the object being overwritten. This was because the struct used
by the ETHTOOL_GFEATURES command of SIOCETHTOOL ends with a 0-length
array, but we were telling ethtool that it could use 2 elements on the
array.
The fix is to allocate the necessary memory with VIR_ALLOC_VAR(),
including the extra length needed for a 2 element array at the end.
---
V2 difference: eliminate all the other cleanups that were in V1, just
fix the crash. I had misunderstood that a failure of the ioctl
shouldn't really be considered a *failure*, and don't have time to
tweak around with all the inconsequential stuff right now.
src/util/virnetdev.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
Looks reasonable to me and since it fixes the crash... Not sure how I
missed the static stack variable in the original patch (and only the dog
has been wearing the cone of shame recently ;-))
ACK
John
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 1e20789..c158ca3 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3130,7 +3130,7 @@ virNetDevGetFeatures(const char *ifname,
size_t i = -1;
struct ethtool_value cmd = { 0 };
# if HAVE_DECL_ETHTOOL_GFEATURES
-struct ethtool_gfeatures g_cmd = { 0 };
+struct ethtool_gfeatures *g_cmd;
# endif
struct elem{
const int cmd;
@@ -3188,10 +3188,14 @@ virNetDevGetFeatures(const char *ifname,
# endif
# if HAVE_DECL_ETHTOOL_GFEATURES
-g_cmd.cmd = ETHTOOL_GFEATURES;
-g_cmd.size = GFEATURES_SIZE;
-if (virNetDevGFeatureAvailable(ifname, g_cmd))
+if (VIR_ALLOC_VAR(g_cmd,
+ struct ethtool_get_features_block, GFEATURES_SIZE) 0)
+return -1;
+g_cmd-cmd = ETHTOOL_GFEATURES;
+g_cmd-size = GFEATURES_SIZE;
+if (virNetDevGFeatureAvailable(ifname, g_cmd))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
+VIR_FREE(g_cmd);
# endif
if (virNetDevRDMAFeature(ifname, out) 0)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: fix qemuDomainSupportsPCI() for ARM machines of virt machinetype
Commit e8d5517 updated the domain post-parse to automatically add
pcie-root et al for certain ARM virt machinetypes, but didn't update
the function qemuDomainSupportsPCI() which is called later on when we
are auto-assigning PCI addresses and default settings for the PCI
controller model and target attributes. The result was that PCI
addresses weren't assigned, and the controllers didn't have their
attribute default values set, leading to an error when the domain was
started, e.g.:
internal error: autogenerated dmi-to-pci-bridge options not set
This patch duplicates the check made in the earlier patch, so that PCI
address auto-assignment and target/model default values will be set.
---
The example Cole gave in his email reporting this bug is fixed by this
patch.
src/qemu/qemu_command.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ae03618..84cbfe1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1742,7 +1742,7 @@ qemuCollectPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
}
static bool
-qemuDomainSupportsPCI(virDomainDefPtr def)
+qemuDomainSupportsPCI(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
{
if ((def-os.arch != VIR_ARCH_ARMV7L) (def-os.arch !=
VIR_ARCH_AARCH64))
return true;
@@ -1750,6 +1750,11 @@ qemuDomainSupportsPCI(virDomainDefPtr def)
if (STREQ(def-os.machine, versatilepb))
return true;
+if ((STREQ(def-os.machine, virt) ||
+ STRPREFIX(def-os.machine, virt-))
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX))
+return true;
+
return false;
}
@@ -2267,7 +2272,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false)))
goto cleanup;
-if (qemuDomainSupportsPCI(def)) {
+if (qemuDomainSupportsPCI(def, qemuCaps)) {
if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) 0)
goto cleanup;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] libvirt-admin: Mark symbols as local
Otherwise we're leaking some 30+ symbols like
virAdmConnectClass
virAdmConnectNew
virConnectClass
virConnectCloseCallbackDataClass
virDomainClass
...
I marked the one symbol needed by the deamon as
LIBVIRT_ADMIN_PRIVATE_VERSION for now.
---
There's likely a better solution for xdr_admin_connect_open_args.
Cheers,
-- Guido
src/libvirt_admin.syms | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/libvirt_admin.syms b/src/libvirt_admin.syms
index d9e3c0b..de4f1b8 100644
--- a/src/libvirt_admin.syms
+++ b/src/libvirt_admin.syms
@@ -16,3 +16,12 @@ LIBVIRT_ADMIN_1.3.0 {
virAdmConnectClose;
virAdmConnectRef;
};
+
+
+LIBVIRT_ADMIN_PRIVATE_1.2.19 {
+global:
+xdr_admin_connect_open_args;
+
+local:
+*;
+};
--
2.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 0/4] qemu: Allow PCI virtio on ARM virt machine
On 08/11/2015 01:51 PM, Cole Robinson wrote: On 07/17/2015 07:27 AM, Pavel Fedin wrote: Virt machine in qemu since v2.3.0 has PCI generic host controller, and can use PCI devices. This provides performance improvement as well as vhost-net with irqfd support for virtio-net. However libvirt currently does not allow ARM virt machine to have PCI devices. This patchset adds the necessary support. Changes since v4: - Rebased onto current master - Added possibility to plug virtio-net-pci adapter directly into PCIe bus. This is necessary for irqfds to work in qemu. Changes since v3: - Capability is based not on qemu version but on support of gpex-pcihost device by qemu - Added a workaround, allowing to pass make check. The problem is that test suite does not build capabilities cache. Unfortunately this means that correct unit-test for the new functionality currently cannot be written. Test suite framework needs to be improved. Changes since v2: Complete rework, use different approach - Correctly model PCI Express bus on the machine. It is now possible to explicitly specify address-type='pci' with attributes. This allows to attach not only virtio, but any other PCI device to the model. - Default is not changed and still mmio, for backwards compatibility with existing installations. PCI bus has to be explicitly specified. - Check for the capability in correct place, in v2 it actually did not work Changes since v1: - Added capability based on qemu version number - Recognize also virt- prefix Pavel Fedin (4): qemu: Introduce QEMU_CAPS_OBJECT_GPEX Add PCI-Express root to ARM virt machine Build correct command line for PCI NICs on ARM Allow to plug virtio-net-pci into PCIe slot I was a bit confused about the patches that landed; I see now that they only add a PCI controller for modern -M virt, but don't change the virtio defaults to use it. This is good, and in my brief testing doesn't cause any regressions with current virt-manager. We can figure out later if/how to change the bus=virtio or model=virtio default to use PCI instead of virtio-mmio But yeah, we need to figure out that test case issue. There's already a regression with git head: $ sudo virsh create test-aarch64.xml error: Failed to create domain from test-aarch64.xml error: internal error: autogenerated dmi-to-pci-bridge options not set Hah! So you see Martin and John, *that* is why I wanted all that extra checking! :-) XML attached. I'm guessing this is one of Laine's patches, CCd Actually the problem is that the original hacks had yet another function added in called qemuDomainSupportsPCI(), which is called during PCI address assignment (also when the PCI controllers are setup), and these new ARM PCI patches haven't added the new sauce to that function. I just added the same bit of code checking for QEMU_CAPS_OBJECT_GPEX, and your example now starts up with no problems. I'm sending the patch now. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
