Re: [Qemu-devel] [PATCH for-2.10 07/16] block/file-posix: Generalize raw_regular_truncate

[Stefan Hajnoczi]

On Wed, Mar 22, 2017 at 05:53:00PM +0100, Max Reitz wrote:
> On 22.03.2017 17:44, Stefan Hajnoczi wrote:
> > On Mon, Mar 20, 2017 at 04:11:24PM +0100, Max Reitz wrote:
> >> On 20.03.2017 12:00, Stefan Hajnoczi wrote:
> >>> On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote:
>  +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t 
>  offset,
> >>>
> >>> The presence of both a file descriptor and a BlockDriverState (actually
> >>> it must be a BDRVRawState) arguments is unusual.  It seems bs is needed
> >>> for bdrv_getlength().
> >>>
> >>> How about using fstat(fd, ) and then dropping bs and create?
> >>
> >> Well, I could do that, but bdrv_getlength() is a bit simpler and I don't
> >> see much of an issue with specifying both an fd and a bs.
> > 
> > Arguments that provide overlapping information make the function harder
> > to understand and use correctly (there are combinations of these
> > arguments that are invalid or don't make sense).  Saving a few lines of
> > code in the function implementation is a poor trade-off IMO.
> 
> My brain likes to agree but for some reason my heart really prefers
> block layer functions (where I know what can go wrong) over POSIX
> functions (where I always have the feeling of maybe not having though of
> everything).
> 
> I guess I'll have to silence my heart for a bit, then.

In matters of the heart I cannot give advice on this mailing list,
sorry.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.10 07/16] block/file-posix: Generalize raw_regular_truncate

[Max Reitz]

On 22.03.2017 17:44, Stefan Hajnoczi wrote:
> On Mon, Mar 20, 2017 at 04:11:24PM +0100, Max Reitz wrote:
>> On 20.03.2017 12:00, Stefan Hajnoczi wrote:
>>> On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote:
 +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t 
 offset,
>>>
>>> The presence of both a file descriptor and a BlockDriverState (actually
>>> it must be a BDRVRawState) arguments is unusual.  It seems bs is needed
>>> for bdrv_getlength().
>>>
>>> How about using fstat(fd, ) and then dropping bs and create?
>>
>> Well, I could do that, but bdrv_getlength() is a bit simpler and I don't
>> see much of an issue with specifying both an fd and a bs.
> 
> Arguments that provide overlapping information make the function harder
> to understand and use correctly (there are combinations of these
> arguments that are invalid or don't make sense).  Saving a few lines of
> code in the function implementation is a poor trade-off IMO.

My brain likes to agree but for some reason my heart really prefers
block layer functions (where I know what can go wrong) over POSIX
functions (where I always have the feeling of maybe not having though of
everything).

I guess I'll have to silence my heart for a bit, then.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.10 07/16] block/file-posix: Generalize raw_regular_truncate

[Stefan Hajnoczi]

On Mon, Mar 20, 2017 at 04:11:24PM +0100, Max Reitz wrote:
> On 20.03.2017 12:00, Stefan Hajnoczi wrote:
> > On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote:
> >> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t 
> >> offset,
> > 
> > The presence of both a file descriptor and a BlockDriverState (actually
> > it must be a BDRVRawState) arguments is unusual.  It seems bs is needed
> > for bdrv_getlength().
> > 
> > How about using fstat(fd, ) and then dropping bs and create?
> 
> Well, I could do that, but bdrv_getlength() is a bit simpler and I don't
> see much of an issue with specifying both an fd and a bs.

Arguments that provide overlapping information make the function harder
to understand and use correctly (there are combinations of these
arguments that are invalid or don't make sense).  Saving a few lines of
code in the function implementation is a poor trade-off IMO.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.10 07/16] block/file-posix: Generalize raw_regular_truncate

[Max Reitz]

On 20.03.2017 12:00, Stefan Hajnoczi wrote:
> On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote:
>> Currently, raw_regular_truncate() is intended for setting the size of a
>> newly created file. However, we also want to use it for truncating an
>> existing file in which case only the newly added space (when growing)
>> should be preallocated.
>>
>> This also means that if resizing failed, we should try to restore the
>> original file size. This is important when using preallocation.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/file-posix.c | 73 
>> --
>>  1 file changed, 60 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 35a9e43f3e..cd229324ba 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1384,11 +1384,39 @@ static void raw_close(BlockDriverState *bs)
>>  }
>>  }
>>  
>> -static int raw_regular_truncate(int fd, int64_t offset, PreallocMode 
>> prealloc,
>> +/**
>> + * Truncates the given regular file @fd to @offset and, when growing, fills 
>> the
>> + * new space according to @prealloc.
>> + *
>> + * @create must be true iff the file is new. In that case, @bs is ignored. 
>> If
>> + * @create is false, @bs must be valid and correspond to the same file as 
>> @fd.
> 
> Returns: 0 on success, -errno on failure.

Yep, will add.

>> + */
>> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t 
>> offset,
> 
> The presence of both a file descriptor and a BlockDriverState (actually
> it must be a BDRVRawState) arguments is unusual.  It seems bs is needed
> for bdrv_getlength().
> 
> How about using fstat(fd, ) and then dropping bs and create?

Well, I could do that, but bdrv_getlength() is a bit simpler and I don't
see much of an issue with specifying both an fd and a bs.

>> +PreallocMode prealloc, bool create,
>>  Error **errp)
>>  {
>>  int result = 0;
>> -char *buf;
>> +int64_t current_length = 0;
>> +char *buf = NULL;
>> +
>> +assert(create || bs);
>> +if (!create) {
>> +BDRVRawState *s = bs->opaque;
>> +assert(s->fd == fd);
>> +}
>> +
>> +if (!create) {
>> +current_length = bdrv_getlength(bs);
>> +if (current_length < 0) {
>> +error_setg_errno(errp, -current_length,
>> + "Could not inquire current length");
>> +return -current_length;
>> +}
>> +
>> +if (current_length > offset && prealloc != PREALLOC_MODE_OFF) {
>> +return -ENOTSUP;
>> +}
> 
> Missing error_setg().

Indeed! Will fix.

Max




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.10 07/16] block/file-posix: Generalize raw_regular_truncate

[Stefan Hajnoczi]

On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote:
> Currently, raw_regular_truncate() is intended for setting the size of a
> newly created file. However, we also want to use it for truncating an
> existing file in which case only the newly added space (when growing)
> should be preallocated.
> 
> This also means that if resizing failed, we should try to restore the
> original file size. This is important when using preallocation.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/file-posix.c | 73 
> --
>  1 file changed, 60 insertions(+), 13 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 35a9e43f3e..cd229324ba 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1384,11 +1384,39 @@ static void raw_close(BlockDriverState *bs)
>  }
>  }
>  
> -static int raw_regular_truncate(int fd, int64_t offset, PreallocMode 
> prealloc,
> +/**
> + * Truncates the given regular file @fd to @offset and, when growing, fills 
> the
> + * new space according to @prealloc.
> + *
> + * @create must be true iff the file is new. In that case, @bs is ignored. If
> + * @create is false, @bs must be valid and correspond to the same file as 
> @fd.

Returns: 0 on success, -errno on failure.

> + */
> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset,

The presence of both a file descriptor and a BlockDriverState (actually
it must be a BDRVRawState) arguments is unusual.  It seems bs is needed
for bdrv_getlength().

How about using fstat(fd, ) and then dropping bs and create?

> +PreallocMode prealloc, bool create,
>  Error **errp)
>  {
>  int result = 0;
> -char *buf;
> +int64_t current_length = 0;
> +char *buf = NULL;
> +
> +assert(create || bs);
> +if (!create) {
> +BDRVRawState *s = bs->opaque;
> +assert(s->fd == fd);
> +}
> +
> +if (!create) {
> +current_length = bdrv_getlength(bs);
> +if (current_length < 0) {
> +error_setg_errno(errp, -current_length,
> + "Could not inquire current length");
> +return -current_length;
> +}
> +
> +if (current_length > offset && prealloc != PREALLOC_MODE_OFF) {
> +return -ENOTSUP;
> +}

Missing error_setg().


signature.asc
Description: PGP signature