[libvirt] [PATCH 0/3] Some minor IOThread API adjustments/checks
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
Allow 0 as an iothread_id and force the hypervisor to handle. The qemuDomainPinIOThread API will look up the iothread_id of 0 and not find it and message that anyway. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt-domain.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index dc89bab..9ec66df 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7926,7 +7926,6 @@ virDomainPinIOThread(virDomainPtr domain, conn = domain->conn; virCheckReadOnlyGoto(conn->flags, error); - virCheckPositiveArgGoto(iothread_id, error); virCheckNonNullArgGoto(cpumap, error); virCheckPositiveArgGoto(maplen, error); -- 2.1.0
The comment for the function indicated that iothread_id had to be a positive non-zero value; however, that wasn't checked - that is a value of 0 is/was allowed by the API and was left up to the hypervisor to reject the value. More than likely this nuance was missed during the many "adjustments" to the API in the review phase. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt-domain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 9ec66df..405962d 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7952,9 +7952,9 @@ virDomainPinIOThread(virDomainPtr domain, * @iothread_id: the specific IOThread ID value to add * @flags: bitwise-OR of virDomainModificationImpact * - * Dynamically add an IOThread to the domain. If @iothread_id is a positive - * non-zero value, then attempt to add the specific IOThread ID and error - * out if the iothread id already exists. + * Dynamically add an IOThread to the domain. It is left up to the + * underlying virtual hypervisor to determine the valid range for an + * @iothread_id and determining whether the @iothread_id already exists. * * Note that this call can fail if the underlying virtualization hypervisor * does not support it or if growing the number is arbitrarily limited. -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1251886 Since iothread_id == 0 is an invalid value for QEMU let's point that out specifically. For the IOThreadDel code, the failure would have ended up being a failure to find the IOThread ID; however, for the IOThreadAdd code - an IOThread 0 was added and that isn't good. It seems during many reviews/edits to the code the check for iothread_id = 0 being invalid was lost - it could have originally been in the API code, but requested to be moved - I cannot remember. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2e44500..7b21ce7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6126,6 +6126,12 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver, virDomainDefPtr persistentDef; int ret = -1; + if (iothread_id == 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid value of 0 for iothread_id")); + return -1; + } + cfg = virQEMUDriverGetConfig(driver); priv = vm->privateData; -- 2.1.0
On 13.08.2015 17:00, John Ferlan wrote:
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(-)
ACK series. Michal
participants (2)
-
John Ferlan -
Michal Privoznik