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

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

Posted by Julio Faracco 6 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
index 8265a39..1359dba 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;
@@ -701,6 +702,9 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
     cbData.ctl = ctl;
     cbData.fd = fd;
 
+    if (virStorageVolGetInfo(vol, &volumeInfo) < 0)
+        goto cleanup;
+
     if (vshCommandOptBool(cmd, "sparse"))
         flags |= VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM;
 
@@ -709,6 +713,11 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
         goto cleanup;
     }
 
+    if (volumeInfo.capacity <= virFileLength(file, fd)) {
+        vshError(ctl, _("file is bigger than volume %s capacity"), name);
+        goto cleanup;
+    }
+
     if (virStorageVolUpload(vol, st, offset, length, flags) < 0) {
         vshError(ctl, _("cannot upload to volume %s"), name);
         goto cleanup;
-- 
2.7.4

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

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

Posted by Peter Krempa 6 years, 3 months ago

On Sun, Jan 07, 2018 at 21:31:24 -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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index 8265a39..1359dba 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;
> @@ -701,6 +702,9 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>      cbData.ctl = ctl;
>      cbData.fd = fd;
>  
> +    if (virStorageVolGetInfo(vol, &volumeInfo) < 0)
> +        goto cleanup;
> +
>      if (vshCommandOptBool(cmd, "sparse"))
>          flags |= VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM;
>  
> @@ -709,6 +713,11 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
>          goto cleanup;
>      }
>  
> +    if (volumeInfo.capacity <= virFileLength(file, fd)) {

I'm not sure that checking whether it's "equal" is a great idea.
Especially since it should exactly fit in that case.

> +        vshError(ctl, _("file is bigger than volume %s capacity"), name);
> +        goto cleanup;
> +    }
> +
>      if (virStorageVolUpload(vol, st, offset, length, flags) < 0) {
>          vshError(ctl, _("cannot upload to volume %s"), name);
>          goto cleanup;
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Peter Krempa 6 years, 3 months ago

On Mon, Jan 08, 2018 at 13:55:10 +0100, Peter Krempa wrote:
> On Sun, Jan 07, 2018 at 21:31:24 -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 | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> > index 8265a39..1359dba 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;
> > @@ -701,6 +702,9 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
> >      cbData.ctl = ctl;
> >      cbData.fd = fd;
> >  
> > +    if (virStorageVolGetInfo(vol, &volumeInfo) < 0)
> > +        goto cleanup;
> > +
> >      if (vshCommandOptBool(cmd, "sparse"))
> >          flags |= VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM;
> >  
> > @@ -709,6 +713,11 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd)
> >          goto cleanup;
> >      }
> >  
> > +    if (volumeInfo.capacity <= virFileLength(file, fd)) {
> 
> I'm not sure that checking whether it's "equal" is a great idea.
> Especially since it should exactly fit in that case.

Also due to the implementation of virFileLength:

off_t
virFileLength(const char *path, int fd)
{
    struct stat s;

    if (fd >= 0) {
        if (fstat(fd, &s) < 0)
            return -1;
    } else {
        if (stat(path, &s) < 0)
            return -1;
    }

    if (!S_ISREG(s.st_mode))
       return -1;

    return s.st_size;

The test of S_ISREG will break upload from non-regular files.

Additionaly the virsh implementation has a parameter --length (stored in
'length' variable, which is not checked in this case. If the user wishes
to upload less than the fill file it would break too.

Also it's not checking that the size + offset fit in the volume either.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list