Hi, Eric. Thank you for merging my patch. On Mon, 14 Mar 2011 21:04:26 -0600 Eric Blake <eblake@xxxxxxxxxx> wrote: > On 03/08/2011 04:06 AM, Minoru Usui wrote: > > This patch changes zerobuf variable from array to VIR_ALLOC_N(). > > > > Signed-off-by: Minoru Usui <usui@xxxxxxxxxxxxxxxxx> > > Thanks for contributing! This looks like your first patch in, so I > updated AUTHORS; let me know if I need to adjust any spelling. Thanks for updating AUTHORS. Above spelling of Signed-off-by is correct. But..., I sent another patch to you and libvirt-devel before. Please check it, if you have any time. http://www.mail-archive.com/libvir-list@xxxxxxxxxx/msg32801.html > > --- > > src/storage/storage_backend.c | 38 +++++++++++++++++++++++++++++--------- > > 1 files changed, 29 insertions(+), 9 deletions(-) > > > > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > > index 2eede74..3a22da4 100644 > > --- a/src/storage/storage_backend.c > > +++ b/src/storage/storage_backend.c > > @@ -36,6 +36,10 @@ > > #include <sys/stat.h> > > #include <sys/param.h> > > #include <dirent.h> > > +#ifdef __linux__ > > +#include <sys/ioctl.h> > > +#include <linux/fs.h> > > +#endif > > This needs indentation tweaking to keep 'make syntax-check' happy when > cppi is installed. Thank you for telling me about it. I agree. > > +#define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) > > +#define WRITE_BLOCK_SIZE_DEFAULT (512) > > Technically, you don't need () around a single integer constant, but it > didn't hurt to leave them in. Why isn't this 4k, though? I don't know 4k is appropriate value for all platform. So I don't change 512. If 4k is best, I think default value changes from 512 to 4k, too. > > +#ifdef __linux__ > > + if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { > > + wbytes = WRITE_BLOCK_SIZE_DEFAULT; > > + } > > +#endif > > What's wrong with fstat(fd) and using st.st_blksize on all other > platforms where the ioctl doesn't exist? That may still be too small on > some platforms, but it's more likely to be write than hard-coding things > at 512 (for example, on Cygwin, st.st_blksize is 64k, which is indeed > the optimal size for NTFS drives). Also, I don't know of any file > system that supports sparse files with holes smaller than st_blksize. I agree with you. Your code looks good for me. > Here's what I squashed in before pushing: > > diff --git i/AUTHORS w/AUTHORS > index c0c1780..0e56468 100644 > --- i/AUTHORS > +++ w/AUTHORS > diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c > index b3eba10..fc08c68 100644 > --- i/src/storage/storage_backend.c > +++ w/src/storage/storage_backend.c > @@ -1,7 +1,7 @@ > /* > * storage_backend.c: internal storage driver backend contract > * > - * Copyright (C) 2007-2010 Red Hat, Inc. > + * Copyright (C) 2007-2011 Red Hat, Inc. > * Copyright (C) 2007-2008 Daniel P. Berrange > * > * This library is free software; you can redistribute it and/or > @@ -37,8 +37,8 @@ > #include <sys/param.h> > #include <dirent.h> > #ifdef __linux__ > -#include <sys/ioctl.h> > -#include <linux/fs.h> > +# include <sys/ioctl.h> > +# include <linux/fs.h> > #endif > > #if HAVE_SELINUX > @@ -113,7 +113,7 @@ enum { > }; > > #define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) > -#define WRITE_BLOCK_SIZE_DEFAULT (512) > +#define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024) > > static int ATTRIBUTE_NONNULL (2) > virStorageBackendCopyToFD(virStorageVolDefPtr vol, > @@ -127,10 +127,11 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, > int ret = 0; > unsigned long long remain; > size_t rbytes = READ_BLOCK_SIZE_DEFAULT; > - size_t wbytes = WRITE_BLOCK_SIZE_DEFAULT; > + size_t wbytes = 0; > int interval; > char *zerobuf; > char *buf = NULL; > + struct stat st; > > if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) { > ret = -errno; > @@ -142,9 +143,13 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, > > #ifdef __linux__ > if (ioctl(fd, BLKBSZGET, &wbytes) < 0) { > - wbytes = WRITE_BLOCK_SIZE_DEFAULT; > + wbytes = 0; > } > #endif > + if ((wbytes == 0) && fstat(fd, &st) == 0) > + wbytes = st.st_blksize; > + if (wbytes < WRITE_BLOCK_SIZE_DEFAULT) > + wbytes = WRITE_BLOCK_SIZE_DEFAULT; > > if (VIR_ALLOC_N(zerobuf, wbytes) < 0) { > ret = -errno; > > > > -- > Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 > Libvirt virtualization library http://libvirt.org > -- Minoru Usui <usui@xxxxxxxxxxxxxxxxx> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list