[libvirt] Correct a check for capacity arg of storageVolumeResize()
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> --- src/storage/storage_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index df0e291..641944d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1758,7 +1758,7 @@ storageVolumeResize(virStorageVolPtr obj, goto out; } - if (abs_capacity > vol->allocation + pool->def->available) { + if (abs_capacity > vol->capacity + pool->def->available) { virStorageReportError(VIR_ERR_INVALID_ARG, _("Not enough space left on storage pool")); goto out; -- 1.7.7.6
On 02/23/2012 08:00 PM, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
--- src/storage/storage_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index df0e291..641944d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1758,7 +1758,7 @@ storageVolumeResize(virStorageVolPtr obj, goto out; }
- if (abs_capacity > vol->allocation + pool->def->available) { + if (abs_capacity > vol->capacity + pool->def->available) {
I'm not sure this is right. If you allow for sparse files and over-commitment of capacity, then sparsely resizing to something that has too much capacity but still fits within allocation limits would be a reasonable that should not be rejected. I think it is more likely that we have several bugs in this area, in being too lax about which values we are actually checking. In fact, is the only reason we are checking for overflow is to avoid wasting time doing a partial allocation just to learn at the end that we would hit ENOSPACE? Adding to your commit message with the actual scenario you used to trigger the problem, and why your fix is correct, will go a long way in convincing me that this is needed.
virStorageReportError(VIR_ERR_INVALID_ARG,
Meanwhile, I just noticed this error code is wrong - the argument was syntactically valid, so the real problem is that we are out of space, which fits better as VIR_ERR_OPERATION_FAILED, or possibly a new error message specific to out-of-space errors. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Feb 24, 2012 at 8:58 PM, Eric Blake <eblake@redhat.com> wrote:
On 02/23/2012 08:00 PM, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
--- src/storage/storage_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index df0e291..641944d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1758,7 +1758,7 @@ storageVolumeResize(virStorageVolPtr obj, goto out; }
- if (abs_capacity > vol->allocation + pool->def->available) { + if (abs_capacity > vol->capacity + pool->def->available) {
I'm not sure this is right. If you allow for sparse files and over-commitment of capacity, then sparsely resizing to something that has too much capacity but still fits within allocation limits would be a reasonable that should not be rejected. I think it is more likely that we have several bugs in this area, in being too lax about which values we are actually checking. In fact, is the only reason we are checking for overflow is to avoid wasting time doing a partial allocation just to learn at the end that we would hit ENOSPACE?
Adding to your commit message with the actual scenario you used to trigger the problem, and why your fix is correct, will go a long way in convincing me that this is needed.
I got a volume with '1G' allocation and '10G' capacity. The available space in the parent pool is '5G'. With the current check for overcapacity, I can only try to resize to <= '6G'. You see the problem?
virStorageReportError(VIR_ERR_INVALID_ARG,
Meanwhile, I just noticed this error code is wrong - the argument was syntactically valid, so the real problem is that we are out of space, which fits better as VIR_ERR_OPERATION_FAILED, or possibly a new error message specific to out-of-space errors.
K, i'll submit a fix for that. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
participants (2)
-
Eric Blake -
Zeeshan Ali (Khattak)