qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 04/19] nbd/server: Hoist length check to qemp


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH v3 04/19] nbd/server: Hoist length check to qemp_nbd_server_add
Date: Tue, 15 Jan 2019 16:26:23 +0000

15.01.2019 18:25, Eric Blake wrote:
> On 1/15/19 3:44 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.01.2019 20:57, Eric Blake wrote:
>>> We only had two callers to nbd_export_new; qemu-nbd.c always
>>> passed a valid offset/length pair (because it already checked
>>> the file length, to ensure that offset was in bounds), while
>>> blockdev-nbd always passed 0/-1.  Then nbd_export_new reduces
>>> the size to a multiple of BDRV_SECTOR_SIZE (can only happen
>>> when offset is not sector-aligned, since bdrv_getlength()
>>> currently rounds up), which can result in offset being greater
>>> than the enforced length, but that's not fatal (the server
>>> rejects client requests that exceed the advertised length).
>>>
>>> However, I'm finding it easier to work with the code if we are
>>> consistent on having both callers pass in a valid length, and
>>> just assert that things are sane in nbd_export_new.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
> 
>>> +++ b/nbd/server.c
>>> @@ -1499,13 +1499,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, 
>>> off_t dev_offset, off_t size,
>>>        exp->name = g_strdup(name);
>>>        exp->description = g_strdup(description);
>>>        exp->nbdflags = nbdflags;
>>> -    exp->size = size < 0 ? blk_getlength(blk) : size;
>>> -    if (exp->size < 0) {
>>> -        error_setg_errno(errp, -exp->size,
>>> -                         "Failed to determine the NBD export's length");
>>> -        goto fail;
>>> -    }
>>> -    exp->size -= exp->size % BDRV_SECTOR_SIZE;
>>> +    assert(dev_offset <= size);
>>
>> @size is not size of the image, but size of the export, so it may be less 
>> than dev_offset
>> (qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, 
>> dev_offset, fd_size, "
> 
> But the assert is fine because patch 3/19 fixed qemu-nbd.c to never pass
> in dev_offset larger than size (it fails up front if dev_offset is out
> of bounds, whether from the -o command line option or from what it read
> from the partition header with the -P command line option).
> 

Don't follow =(

Assume, image size 3M, and we have offset 2M, i.e. -o 2M.

than in qemu-nbd.c, we have

fd_size = blk_getlength(blk); # 3M
...
fd_size -= dev_offset; # 1M
...
export = nbd_export_new(bs, dev_offset, fd_size # bs, 2M, 1M

in nbd_export_new:

assert(dev_offset <= size); # 2M <= 1M

fail.

-- 
Best regards,
Vladimir

reply via email to

[Prev in Thread] Current Thread [Next in Thread]