- Devel - libvirt List Archives

[libvirt] [PATCH 0/5] Make sure internal blkiotune values are in sync
by Martin Kletzander 19 Aug '15

19 Aug '15

We were blindly setting blkiotune values for devices, but kernel can throw some of them away. This series reworks the logic the same wayother tuning values are updated. That is, after the value gets set, it is read back again to make sure internal structures are in sync and we can return the values from them. Little example should clear up everything. Before: ======= $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551614 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: /dev/sda,18446744073709551614 device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device 8:0 18446744073709551614 $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551615 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: /dev/sda,18446744073709551615 device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device $ After: ====== $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551614 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: /dev/sda,18446744073709551614 device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device 8:0 18446744073709551614 $ virsh blkiotune dummy --device-read-bytes-sec /dev/sda,18446744073709551615 $ virsh blkiotune dummy weight : 500 device_weight : device_read_iops_sec: device_write_iops_sec: device_read_bytes_sec: device_write_bytes_sec: $ cat /sys/fs/cgroup/blkio/machine/dummy.libvirt-qemu/blkio.throttle.read_bps_device $ Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165580 Martin Kletzander (5): util: Add virStringGetFirstWithPrefix util: Add virCgroupGetBlockDevString util: Add getters for cgroup block device I/O throttling lxc: Sync BlkioDevice values when setting them in cgroups qemu: Sync BlkioDevice values when setting them in cgroups src/libvirt_private.syms | 6 + src/lxc/lxc_cgroup.c | 20 ++- src/lxc/lxc_driver.c | 25 ++- src/qemu/qemu_cgroup.c | 20 ++- src/qemu/qemu_driver.c | 25 ++- src/util/vircgroup.c | 457 +++++++++++++++++++++++++++++++++++------------ src/util/vircgroup.h | 20 +++ src/util/virstring.c | 17 ++ src/util/virstring.h | 2 + 9 files changed, 459 insertions(+), 133 deletions(-) -- 2.5.0

2 8

[libvirt] [PATCH] qemuDomainRename: Don't leave a domain locked uppon fail
by Michal Privoznik 18 Aug '15

18 Aug '15

Well, yet again one case of 'goto cleanup' while 'goto endjob' was needed. Sorry. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Pushed as trivial. src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 180dd88..99a3817 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19943,7 +19943,7 @@ static int qemuDomainRename(virDomainPtr dom, if (STREQ(vm->def->name, new_name)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Can't rename domain to itself")); - goto cleanup; + goto endjob; } if (VIR_STRDUP(new_dom_name, new_name) < 0) -- 2.4.6

1 0

[libvirt] [PATCH 0/3] Some minor IOThread API adjustments/checks
by John Ferlan 18 Aug '15

18 Aug '15

As a result of the review of a related issue for virDomainAddIOThread it was noted that it's only our implementation for qemu that doesn't want an iothread_id == 0, see: http://www.redhat.com/archives/libvir-list/2015-August/msg00310.html Continued IRC chats with the submittor of that patch discovered some inconsistencies which are dealt with in this patch series... Patch 1 - Remove the iothread_id arg check in virDomainPinIOThread. Since 0 was allowed by the API anyway and left up to the hypervisor. The argument is unsigned - so in reality all that was being checked was 0 - other similar callers work on 'int' values not 'unsigned int'. Patch 2 - Adjust the description in virDomainAddIOThread to remove the "positive non-zero value" and then rework the text to be clearer that it's up to the hypervisor to decide what values are legal and how to handle possible duplication. Patch 3 - Add check in qemu_driver Add/Del IOThread API for the invalid 0 value for iothread_id NOTE: It's not necessary to add a specific illegal argument check for iothread_id = 0 in qemuDomainPinIOThread since both the live and config paths will first search for the iothread_id value and fail if not found in the iothreadid list. I'm not opposed to adding a specific error message if so desired... John Ferlan (3): api: Remove check on iothread_id arg in virDomainPinIOThread api: Adjust comment for virDomainAddIOThread qemu: Add check for invalid iothread_id in qemuDomainChgIOThread src/libvirt-domain.c | 7 +++---- src/qemu/qemu_driver.c | 6 ++++++ 2 files changed, 9 insertions(+), 4 deletions(-) -- 2.1.0

2 4

[libvirt] [PATCH 0/2] 2 small fix for domrename
by Luyao Huang 18 Aug '15

18 Aug '15

*** BLURB HERE *** Luyao Huang (2): virsh: fix always return false in domrename libvirt-domain: forbid use virDomainRename in readonly connection src/libvirt-domain.c | 1 + tools/virsh-domain.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) -- 1.8.3.1

3 4

[libvirt] [PATCH] qemuDomainRename: Report error if adding domain to domainObj list fails
by Michal Privoznik 18 Aug '15

18 Aug '15

So, domain renaming works like this: new domain name is added into the list of domain objects. Then, domain definition is updated. After that, old domain name is removed from the domain object list. Now, if the very firs step fails for some reason, no error is reported: virsh # domrename dummy dummy error: An error occurred, but the cause is unknown Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3683591..8e365bd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19950,8 +19950,11 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; } - if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) + if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("could not add new name into internal list of domains")); goto endjob; + } if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) goto rollback; -- 2.4.6

2 7

[libvirt] [PATCH] build: fix mingw build
by Eric Blake 18 Aug '15

18 Aug '15

Ever since commit e44b0269, 64-bit mingw compilation fails with: ../../src/util/virprocess.c: In function 'virProcessGetPids': ../../src/util/virprocess.c:628:50: error: passing argument 4 of 'virStrToLong_i' from incompatible pointer type [-Werror=incompatible-pointer-types] if (virStrToLong_i(ent->d_name, NULL, 10, &tmp_pid) < 0) ^ In file included from ../../src/util/virprocess.c:59:0: ../../src/util/virstring.h:53:5: note: expected 'int *' but argument is of type 'pid_t * {aka long long int *}' int virStrToLong_i(char const *s, ^ cc1: all warnings being treated as errors Although mingw won't be using this function, it does compile the file, and the fix is relatively simple. * src/util/virprocess.c (virProcessGetPids): Don't assume pid_t fits in int. Signed-off-by: Eric Blake <eblake(a)redhat.com> --- Pushing under build-breaker rule. src/util/virprocess.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 8fa7a9b..77a038a 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -619,14 +619,16 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids) goto cleanup; while ((value = virDirRead(dir, &ent, taskPath)) > 0) { + long long tmp; pid_t tmp_pid; /* Skip . and .. */ if (STRPREFIX(ent->d_name, ".")) continue; - if (virStrToLong_i(ent->d_name, NULL, 10, &tmp_pid) < 0) + if (virStrToLong_ll(ent->d_name, NULL, 10, &tmp) < 0) goto cleanup; + tmp_pid = tmp; if (VIR_APPEND_ELEMENT(*pids, *npids, tmp_pid) < 0) goto cleanup; -- 2.4.3

1 0

[libvirt] [PATCH v2 0/3] Some additional checks for virDomainRename
by Michal Privoznik 17 Aug '15

17 Aug '15

So after some discussion to my original approach, this is v2. Michal Privoznik (3): virHashAddOrUpdateEntry: Tunr @new_name into void * virHashAddEntry: Report error on duplicate key qemuDomainRename: Explicitly check if domain is renaming to itself src/qemu/qemu_driver.c | 6 ++++++ src/util/virhash.c | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) -- 2.4.6

2 6

[libvirt] [PATCH] utils: Remove the logging of errors from virNetDevSendEthtoolIoctl
by Moshe Levi 17 Aug '15

17 Aug '15

This patch remove the logging of errors of ioctl api and instead let the caller to choose what errors to log --- src/util/virnetdev.c | 44 +++++++++++++------------------------------- 1 files changed, 13 insertions(+), 31 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 2f3690e..cf79e8d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -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 ((sock = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) { virReportSystemError(errno, "%s", _("Cannot open control socket")); goto cleanup; } @@ -3045,26 +3044,9 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) strcpy(ifr.ifr_name, ifname); ifr.ifr_data = cmd; ret = ioctl(sock, SIOCETHTOOL, &ifr); - if (ret != 0) { - switch (errno) { - case EPERM: - VIR_DEBUG("ethtool ioctl: permission denied"); - break; - case EINVAL: - VIR_DEBUG("ethtool ioctl: invalid request"); - break; - case EOPNOTSUPP: - VIR_DEBUG("ethtool ioctl: request not supported"); - break; - default: - virReportSystemError(errno, "%s", _("ethtool ioctl error")); - goto cleanup; - } - } cleanup: - if (sock) - VIR_FORCE_CLOSE(sock); + VIR_FORCE_CLOSE(sock); return ret; } @@ -3081,12 +3063,12 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) static int virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) { - int ret = -1; - cmd = (void*)cmd; - if (!virNetDevSendEthtoolIoctl(ifname, cmd)) - ret = cmd->data > 0 ? 1 : 0; - return ret; + if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) { + virReportSystemError(errno, _("Cannot get device %s flags"), ifname); + return -1; + } + return cmd->data > 0 ? 1 : 0; } @@ -3103,12 +3085,12 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) static int virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) { - int ret = -1; - cmd = (void*)cmd; - if (!virNetDevSendEthtoolIoctl(ifname, cmd)) - ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); - return ret; + if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) { + virReportSystemError(errno, _("Cannot get device %s generic features"), ifname); + return -1; + } + return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); } # endif -- 1.7.1

2 4

[libvirt] [PATCH] qemu: Resolve Coverity UNINIT
by John Ferlan 16 Aug '15

16 Aug '15

Coverity complained that 'vm' wasn't initialized before jumping to cleanup: and calling virDomainObjEndAPI if the VIR_STRDUP fails. Rather than initialize vm = NULL, I moved the VIR_STRDUP closer to usage and used endjob for goto. Signed-off-by: John Ferlan <jferlan(a)redhat.com> --- src/qemu/qemu_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3683591..a54a3dd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19902,9 +19902,6 @@ static int qemuDomainRename(virDomainPtr dom, virCheckFlags(0, ret); - if (VIR_STRDUP(new_dom_name, new_name) < 0) - goto cleanup; - if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -19940,6 +19937,9 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; } + if (VIR_STRDUP(new_dom_name, new_name) < 0) + goto endjob; + if (virAsprintf(&rename_log_msg, ": domain %s has been renamed to %s\n", vm->def->name, new_name) < 0) { goto endjob; -- 2.1.0

2 2

[libvirt] [PATCH] protocol: Don't use rename as a variable name
by Martin Kletzander 15 Aug '15

15 Aug '15

That causes shadowing errors on older compilers. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com> --- Pushed under the build-breaker rule. src/remote/remote_protocol.x | 2 +- src/remote_protocol-structs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 770aa72dc0f6..92a92e2bfa24 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3237,7 +3237,7 @@ struct remote_domain_rename_args { }; struct remote_domain_rename_ret { - int rename; + int retcode; }; /*----- Protocol. -----*/ diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index ca36dc96e634..ff99c0096941 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2690,7 +2690,7 @@ struct remote_domain_rename_args { u_int flags; }; struct remote_domain_rename_ret { - int rename; + int retcode; }; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, -- 2.5.0

1 0