[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v4 3/4] raw-posix: Add full image preallocat
From: |
Hu Tao |
Subject: |
Re: [Qemu-devel] [RFC PATCH v4 3/4] raw-posix: Add full image preallocation option |
Date: |
Mon, 20 Jan 2014 10:19:58 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Jan 17, 2014 at 04:25:14PM +0800, Stefan Hajnoczi wrote:
> On Fri, Dec 27, 2013 at 11:05:53AM +0800, Hu Tao wrote:
> > This patch adds a new option preallocation for raw format, and implements
> > full preallocation.
> >
> > Signed-off-by: Hu Tao <address@hidden>
> > ---
> > block/raw-posix.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> >
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 6f6b8c1..a722d27 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -1160,17 +1160,52 @@ static int64_t
> > raw_get_allocated_file_size(BlockDriverState *bs)
> > return (int64_t)st.st_blocks * 512;
> > }
> >
> > +#ifdef __linux__
> > +static int raw_preallocate2(int fd, int64_t offset, int64_t length)
>
> Why is this function called raw_preallocate2()? Please choose a
> descriptive name.
Sure.
>
> > +{
> > + int ret = -1;
> > +
> > + ret = fallocate(fd, 0, offset, length);
> > + if (ret < 0) {
> > + ret = -errno;
> > + }
> > +
> > + /* fallback to posix_fallocate() if fallocate() is not supported */
> > + if (ret == -ENOSYS || ret == -EOPNOTSUPP) {
> > + ret = posix_fallocate(fd, offset, length);
>
> From the posix_fallocate(3) man page:
> "posix_fallocate() returns zero on success, or an error number on
> failure. Note that errno is not set."
>
> You need to negate the error number:
> if (ret > 0) {
> ret = -ret; /* posix_fallocate(3) returns a positive errno */
> }
Sure.
>
> > + }
> > +
> > + return ret;
> > +}
> > +#else
> > +static int raw_preallocate2(int fd, int64_t offset, int64_t length)
> > +{
> > + return posix_fallocate(fd, offset, length);
>
> Same error number problem as above.
>
> > @@ -1185,6 +1220,12 @@ static int raw_create(const char *filename,
> > QEMUOptionParameter *options,
> > result = -errno;
> > error_setg_errno(errp, -result, "Could not resize file");
> > }
> > + if (prealloc == PREALLOC_MODE_FULL) {
> > + result = raw_preallocate2(fd, 0, total_size);
>
> What if ftruncate() failed? We should not try to fallocate. Please add
> a proper error return path.
Sure.
>
> > + if (result != 0)
> > + error_setg_errno(errp, -result,
> > + "Could not preallocate data for the new
> > file");
>
> WARNING: braces {} are necessary for all arms of this statement
>
> Please run scripts/checkpatch.pl before submitting patches, it checks
> QEMU coding style.
Thanks!