[libvirt] [PATCH] Fix performance problem of virStorageVolCreateXMLFrom()

[Minoru Usui]

Hi, Daniel.

Thank you for your comment.

> > @@ -119,8 +126,10 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
> >      int amtread = -1;
> >      int ret = 0;
> >      unsigned long long remain;
> > -    size_t rbytes = 1024 * 1024; /* For Read */
> > -    char zerobuf[512];
> > +    size_t rbytes = READ_BLOCK_SIZE_DEFAULT;
> > +    size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT;
> > +    int interval;
> > +    char zerobuf[WRITE_BLOCK_SIZE_DEFAULT];
> 
> Here there is a fixed size buffer which will be written...
> 
> >      char *buf = NULL;
> >  
> >      if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) {
> > @@ -131,6 +140,12 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
> >          goto cleanup;
> >      }
> >  
> > +#ifdef __linux__
> > +    if (ioctl(fd, BLKBSZGET, &wbytes) < 0) {
> > +        wbytes = WRITE_BLOCK_SIZE_DEFAULT;
> > +    }
> > +#endif
> 
> ...but the amount written is variable. So this will have an
> out of bounds array access if the returned 'wbytes' is greater
> than WRITE_BLOCK_SIZE_DEFAULT. I think you need to change
> 'zerobuf' to be a heap-allocated variable, instead of fixed
> sized stack allocated variable. eg
> 
>      char *zerobuf;
>      if (VIR_ALLOC_N(zerobuf, wbytes) < 0) {
>          virReportOOMError();
>          goto cleanup;
>      }
>      ....

Ouch, I make a big mistake.
You are right. I'll remake a patch.

> > +
> >      bzero(&zerobuf, sizeof(zerobuf));
> 
> VIR_ALLOC_N zeros, so this line could then be removed.

OK. I'll remove this.

> >  
> >      if (VIR_ALLOC_N(buf, rbytes) < 0) {
> > @@ -160,7 +175,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
> >           * blocks */
> >          amtleft = amtread;
> >          do {
> > -            int interval = ((512 > amtleft) ? amtleft : 512);
> > +            interval = ((wbytes > amtleft) ? amtleft : wbytes);
> >              int offset = amtread - amtleft;
> >  
> >              if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) {
> > @@ -179,7 +194,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
> >                  goto cleanup;
> >  
> >              }
> > -        } while ((amtleft -= 512) > 0);
> > +        } while ((amtleft -= interval) > 0);
> >      }
> >  
> >      if (VIR_CLOSE(inputfd) < 0) {
> 
> 
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


-- 
Minoru Usui <usui at mxm.nes.nec.co.jp>