[v2] virsh: Checking the volume capacity before uploading a new file.

[libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

Posted by Julio Faracco 7 years, 3 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

Posted by Julio Faracco 7 years, 3 months ago

> +    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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

Posted by Peter Krempa 7 years, 3 months ago

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;
> +    }
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

Posted by Julio Faracco 7 years, 3 months ago

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;
>> +    }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] virsh: Checking the volume capacity before uploading a new file.

Posted by Michal Privoznik 7 years, 3 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list