[Qemu-devel] Is the use of bdrv_getlength() in handle_aiocb_write_zeroes() kosher?

[Qemu-devel] Is the use of bdrv_getlength() in handle_aiocb_write_zeroes() kosher?

[Markus Armbruster]

Denis, you added this in commit d50d822:

#ifdef CONFIG_FALLOCATE
    if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
        int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
        if (ret == 0 || ret != -ENOTSUP) {
            return ret;
        }
        s->has_fallocate = false;
    }
#endif

bdrv_getlength() can fail.  Does it do the right thing then?  For what
it's worth, the comparison of its value is signed.

[Qemu-devel] Is the use of bdrv_getlength() in parallels.c kosher? (was: Is the use of bdrv_getlength() in handle_aiocb_write_zeroes() kosher?)

[Markus Armbruster]

Same question for allocate_clusters() in parallels.c, commit 5a41e1f,
modified in commit ddd2ef2:

    if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) {

bdrv_getlength() can fail.  Does it do the right thing then?  For what
it's worth, the comparison of its value is signed.

There's another one in parallels_open():

    if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
                          PREALLOC_MODE_OFF, NULL) != 0) {
        s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
    }

Re: [Qemu-devel] Is the use of bdrv_getlength() in parallels.c kosher?

[Denis V. Lunev]

On 08/04/2017 03:31 PM, Markus Armbruster wrote:
> Same question for allocate_clusters() in parallels.c, commit 5a41e1f,
> modified in commit ddd2ef2:
>
>     if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) {
>
> bdrv_getlength() can fail.  Does it do the right thing then?  For what
> it's worth, the comparison of its value is signed.
>
> There's another one in parallels_open():
>
>     if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) ||
>             bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs),
>                           PREALLOC_MODE_OFF, NULL) != 0) {
>         s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
>     }
the same as in the previous letter. Saniti checks will not harm.
I'll make a patch.

Thank you for the suggestion.

Den

Re: [Qemu-devel] Is the use of bdrv_getlength() in handle_aiocb_write_zeroes() kosher?

[Denis V. Lunev]

On 08/04/2017 03:16 PM, Markus Armbruster wrote:
> Denis, you added this in commit d50d822:
>
> #ifdef CONFIG_FALLOCATE
>     if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
>         int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
>         if (ret == 0 || ret != -ENOTSUP) {
>             return ret;
>         }
>         s->has_fallocate = false;
>     }
> #endif
>
> bdrv_getlength() can fail.  Does it do the right thing then?  For what
> it's worth, the comparison of its value is signed.
fallocate() with 0 flags can work only beyond end of file
or on top of the hole. Thus the check is made to validate
that we are beyond EOF.

Technically fallocate should fail if that condition will be
violated. But you right, we can add sanity check here.
This would not harm.

Should I send it?

Den

Re: [Qemu-devel] Is the use of bdrv_getlength() in handle_aiocb_write_zeroes() kosher?

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> On 08/04/2017 03:16 PM, Markus Armbruster wrote:
>> Denis, you added this in commit d50d822:
>>
>> #ifdef CONFIG_FALLOCATE
>>     if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) {
>>         int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes);
>>         if (ret == 0 || ret != -ENOTSUP) {
>>             return ret;
>>         }
>>         s->has_fallocate = false;
>>     }
>> #endif
>>
>> bdrv_getlength() can fail.  Does it do the right thing then?  For what
>> it's worth, the comparison of its value is signed.
> fallocate() with 0 flags can work only beyond end of file
> or on top of the hole. Thus the check is made to validate
> that we are beyond EOF.
>
> Technically fallocate should fail if that condition will be
> violated. But you right, we can add sanity check here.
> This would not harm.
>
> Should I send it?

I figure an explicit check for bdrv_getlength() failure would make the
code easier to understand.  In short: yes, please!