[libvirt] [PATCH] phyp: fix logic error on volume creation
The phyp code claims that it wants a non-zero value, but actually enforces a capacity of zero. It has been this way since commit ebc46fe in June 2010. Bummer that it has my name as the committer - I guess I should have been much more stubborn about not blindly taking someone else's 1600-line patch. * src/phyp/phyp_driver.c (phypStorageVolCreateXML): Use correct logic. Signed-off-by: Eric Blake <eblake@redhat.com> --- The fact that this bug has gone unnoticed for years makes me wonder if we are better off just removing the phyp driver from our code base, since it is obvious it is not getting much testing. I'm also waiting for a review on this, because although I _think_ the code wanted a non-zero capacity, I don't know enough about phyp and the "viosvrcmd -c 'mklv -lv'" command line; maybe the comments are wrong and it always wanted 0 capacity instead (which is the only thing that would get past the pre-patch code check). src/phyp/phyp_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index fc3e7db..3a5eefd 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2003,15 +2003,15 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, * in the moment you create the volume. * */ if (voldef->key) { VIR_ERROR(_("Key must be empty, Power Hypervisor will create one for you.")); goto err; } - if (voldef->capacity) { + if (!voldef->capacity) { VIR_ERROR(_("Capacity cannot be empty.")); goto err; } key = phypBuildVolume(pool->conn, voldef->name, spdef->name, voldef->capacity); -- 1.9.0
On 04/02/14 01:39, Eric Blake wrote:
The phyp code claims that it wants a non-zero value, but actually enforces a capacity of zero. It has been this way since commit ebc46fe in June 2010. Bummer that it has my name as the committer - I guess I should have been much more stubborn about not blindly taking someone else's 1600-line patch.
* src/phyp/phyp_driver.c (phypStorageVolCreateXML): Use correct logic.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
The fact that this bug has gone unnoticed for years makes me wonder if we are better off just removing the phyp driver from our code base, since it is obvious it is not getting much testing. I'm also waiting for a review on this, because although I _think_ the code wanted a non-zero capacity, I don't know enough about phyp and the "viosvrcmd -c 'mklv -lv'" command line; maybe the comments are wrong and it always wanted 0 capacity instead (which is the only thing that would get past the pre-patch code check).
I leave this dispute for others;
src/phyp/phyp_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index fc3e7db..3a5eefd 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2003,15 +2003,15 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, * in the moment you create the volume. * */ if (voldef->key) { VIR_ERROR(_("Key must be empty, Power Hypervisor will create one for you.")); goto err; }
- if (voldef->capacity) { + if (!voldef->capacity) { VIR_ERROR(_("Capacity cannot be empty.")); goto err; }
key = phypBuildVolume(pool->conn, voldef->name, spdef->name, voldef->capacity);
ACK to this change. Peter
On 04/02/2014 03:49 AM, Peter Krempa wrote:
On 04/02/14 01:39, Eric Blake wrote:
The phyp code claims that it wants a non-zero value, but actually enforces a capacity of zero. It has been this way since commit ebc46fe in June 2010. Bummer that it has my name as the committer - I guess I should have been much more stubborn about not blindly taking someone else's 1600-line patch.
* src/phyp/phyp_driver.c (phypStorageVolCreateXML): Use correct logic.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
The fact that this bug has gone unnoticed for years makes me wonder if we are better off just removing the phyp driver from our code base, since it is obvious it is not getting much testing. I'm also waiting for a review on this, because although I _think_ the code wanted a non-zero capacity, I don't know enough about phyp and the "viosvrcmd -c 'mklv -lv'" command line; maybe the comments are wrong and it always wanted 0 capacity instead (which is the only thing that would get past the pre-patch code check).
I leave this dispute for others;
I did some googling, and found: http://pic.dhe.ibm.com/infocenter/powersys/v3r1m5/topic/iphcg/mklv.htm?resul... which confirms my suspicion that mklv wants a non-zero size argument. I agree with leaving the dispute on whether the phyp driver is worth culling to another day. So for now...
src/phyp/phyp_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index fc3e7db..3a5eefd 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2003,15 +2003,15 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, * in the moment you create the volume. * */ if (voldef->key) { VIR_ERROR(_("Key must be empty, Power Hypervisor will create one for you.")); goto err; }
- if (voldef->capacity) { + if (!voldef->capacity) { VIR_ERROR(_("Capacity cannot be empty.")); goto err; }
key = phypBuildVolume(pool->conn, voldef->name, spdef->name, voldef->capacity);
ACK to this change.
...I've pushed this patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02.04.2014 01:39, Eric Blake wrote:
The phyp code claims that it wants a non-zero value, but actually enforces a capacity of zero. It has been this way since commit ebc46fe in June 2010. Bummer that it has my name as the committer - I guess I should have been much more stubborn about not blindly taking someone else's 1600-line patch.
* src/phyp/phyp_driver.c (phypStorageVolCreateXML): Use correct logic.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
The fact that this bug has gone unnoticed for years makes me wonder if we are better off just removing the phyp driver from our code base, since it is obvious it is not getting much testing. I'm also waiting for a review on this, because although I _think_ the code wanted a non-zero capacity, I don't know enough about phyp and the "viosvrcmd -c 'mklv -lv'" command line; maybe the comments are wrong and it always wanted 0 capacity instead (which is the only thing that would get past the pre-patch code check).
C'mon. Who's really creating volumes via libvirt? I mean, the fact that users don't create volumes via libvirt doesn't mean they are not using phyp driver. I probably should not say this aloud, but I used volume creation only when starting with libvirt. I used to play with virt-manager to get sense of libvirt's capabilities. I admit that things are different now sice we have domains and storage pools tied together, but not for me as I'm already used to doing it the other way. Michal
participants (3)
-
Eric Blake -
Michal Privoznik -
Peter Krempa