[libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.
The current command 'vol-upload' is not checking if the volume accepts a file bigger than its capacity. It can cause an interrupt of the upload stream. This commit adds a check that fails before starting to send new file to volume if the file is bigger. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1529059 Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tools/virsh-volume.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 8265a39..04e480c 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -672,6 +672,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) { const char *file = NULL; virStorageVolPtr vol = NULL; + virStorageVolInfo volumeInfo; bool ret = false; int fd = -1; virStreamPtr st = NULL; @@ -679,6 +680,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; virshControlPtr priv = ctl->privData; unsigned int flags = 0; + off_t fileLen = -1; virshStreamCallbackData cbData; if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) @@ -701,6 +703,29 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) cbData.ctl = ctl; cbData.fd = fd; + if (virStorageVolGetInfo(vol, &volumeInfo) < 0) + goto cleanup; + + if ((fileLen = virFileLength(file, fd)) < 0) { + vshError(ctl, _("cannot get the file %s length"), file); + goto cleanup; + } + + if (length < fileLen) { + vshError(ctl, _("length parameter is smaller than file size")); + goto cleanup; + } + + if (volumeInfo.capacity < length) { + vshError(ctl, _("lenth parameter is bigger than volume %s capacity"), name); + goto cleanup; + } + + if (volumeInfo.capacity < fileLen + offset) { + vshError(ctl, _("file is bigger than volume %s capacity"), name); + goto cleanup; + } + if (vshCommandOptBool(cmd, "sparse")) flags |= VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM; -- 2.7.4
+ if (volumeInfo.capacity < length) { + vshError(ctl, _("lenth parameter is bigger than volume %s capacity"), name); + goto cleanup; + } + + if (volumeInfo.capacity < fileLen + offset) { + vshError(ctl, _("file is bigger than volume %s capacity"), name); + goto cleanup; + } +
I'm not totally right if I split this IF's or join them into a single IF. I'm assuming that more information is better to understand the error.
On Sun, Jan 21, 2018 at 21:51:05 -0200, Julio Faracco wrote:
The current command 'vol-upload' is not checking if the volume accepts a file bigger than its capacity. It can cause an interrupt of the upload stream. This commit adds a check that fails before starting to send new file to volume if the file is bigger.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1529059
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tools/virsh-volume.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 8265a39..04e480c 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -672,6 +672,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) { const char *file = NULL; virStorageVolPtr vol = NULL; + virStorageVolInfo volumeInfo; bool ret = false; int fd = -1; virStreamPtr st = NULL; @@ -679,6 +680,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; virshControlPtr priv = ctl->privData; unsigned int flags = 0; + off_t fileLen = -1; virshStreamCallbackData cbData;
if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) @@ -701,6 +703,29 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) cbData.ctl = ctl; cbData.fd = fd;
+ if (virStorageVolGetInfo(vol, &volumeInfo) < 0) + goto cleanup; + + if ((fileLen = virFileLength(file, fd)) < 0) { + vshError(ctl, _("cannot get the file %s length"), file);
This is wrong since you might want to upload contents of a block device, in which case you'd report this error.
+ goto cleanup; + } + + if (length < fileLen) {
I think this is wrong. The 'length' parameter was designed to allow upload of a partial file. Adding this would break it.
+ vshError(ctl, _("length parameter is smaller than file size")); + goto cleanup; + }
Hi Peter, I agree about "length" parameter. What I didn't understand is this error (check my test case): # virsh vol-create-as --pool disk-loop loop0 --capacity 20M # virsh vol-info loop0 disk-loop Name: loop0 Type: file Capacity: 20.00 MiB Allocation: 20.00 MiB # dd if=/dev/zero of=/tmp/test bs=10M count=1 # ls -l /tmp/test -rw-rw-r-- 1 julio julio 10485760 Jan 23 00:31 /tmp/test Now, let's upload 5M (partial): # virsh vol-upload loop0 /tmp/test --pool disk-loop --length 5242880 error: cannot send data to volume loop0 error: Library function returned error but did not set virError But, if I set a length with 15M, no errors occurred: # virsh vol-upload loop0 /tmp/test --pool disk-loop --length 15728640 2018-01-22 12:44 GMT-02:00 Peter Krempa <pkrempa@redhat.com>:
On Sun, Jan 21, 2018 at 21:51:05 -0200, Julio Faracco wrote:
The current command 'vol-upload' is not checking if the volume accepts a file bigger than its capacity. It can cause an interrupt of the upload stream. This commit adds a check that fails before starting to send new file to volume if the file is bigger.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1529059
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tools/virsh-volume.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 8265a39..04e480c 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -672,6 +672,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) { const char *file = NULL; virStorageVolPtr vol = NULL; + virStorageVolInfo volumeInfo; bool ret = false; int fd = -1; virStreamPtr st = NULL; @@ -679,6 +680,7 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) unsigned long long offset = 0, length = 0; virshControlPtr priv = ctl->privData; unsigned int flags = 0; + off_t fileLen = -1; virshStreamCallbackData cbData;
if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) @@ -701,6 +703,29 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) cbData.ctl = ctl; cbData.fd = fd;
+ if (virStorageVolGetInfo(vol, &volumeInfo) < 0) + goto cleanup; + + if ((fileLen = virFileLength(file, fd)) < 0) { + vshError(ctl, _("cannot get the file %s length"), file);
This is wrong since you might want to upload contents of a block device, in which case you'd report this error.
+ goto cleanup; + } + + if (length < fileLen) {
I think this is wrong. The 'length' parameter was designed to allow upload of a partial file. Adding this would break it.
+ vshError(ctl, _("length parameter is smaller than file size")); + goto cleanup; + }
On 01/23/2018 03:42 AM, Julio Faracco wrote:
Hi Peter,
I agree about "length" parameter. What I didn't understand is this error (check my test case):
# virsh vol-create-as --pool disk-loop loop0 --capacity 20M
# virsh vol-info loop0 disk-loop Name: loop0 Type: file Capacity: 20.00 MiB Allocation: 20.00 MiB
# dd if=/dev/zero of=/tmp/test bs=10M count=1
# ls -l /tmp/test -rw-rw-r-- 1 julio julio 10485760 Jan 23 00:31 /tmp/test
Now, let's upload 5M (partial):
# virsh vol-upload loop0 /tmp/test --pool disk-loop --length 5242880 error: cannot send data to volume loop0 error: Library function returned error but did not set virError
This is a libvirt bug. Very likely introduced by me when writing the sparse streams feature. Please report a bug unless you want to investigate further. My quick glance at the issue suggests that the problem lies in daemonStreamHandleWriteData() and the way end of stream is handled. I mean, daemonStreamHandleWriteData() calls virStreamSend() which in turn calls virFDStreamWrite(). On the last call, fdst->length is equal fdst->offset which causes error. Well, it needs to cause stream finish. BTW: another problem is that virReportSystemError(ENOSPC) is not being propagated to the client. Michal
participants (3)
-
Julio Faracco -
Michal Privoznik -
Peter Krempa