qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export i


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v5 1/9] nbd: Create struct for tracking export info
Date: Thu, 13 Jul 2017 09:35:35 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

On 07/13/2017 07:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.07.2017 15:09, Vladimir Sementsov-Ogievskiy wrote:
>> 07.07.2017 23:30, Eric Blake wrote:
>>> The NBD Protocol is introducing some additional information
>>> about exports, such as minimum request size and alignment, as
>>> well as an advertised maximum request size.  It will be easier
>>> to feed this information back to the block layer if we gather
>>> all the information into a struct, rather than adding yet more
>>> pointer parameters during negotiation.
>>
>> // it was me who do so)
>>

>>> +int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
>>>                Error **errp)
>>>   {
>>> -    unsigned long sectors = size / BDRV_SECTOR_SIZE;
>>> -    if (size / BDRV_SECTOR_SIZE != sectors) {
>>> -        error_setg(errp, "Export size %lld too large for 32-bit
>>> kernel",
>>> -                   (long long) size);
>>> +    unsigned long sectors = info->size / BDRV_SECTOR_SIZE;
>>> +    if (info->size / BDRV_SECTOR_SIZE != sectors) {
>>> +        error_setg(errp, "Export size %" PRId64 " too large for
>>> 32-bit kernel",
>>
>> should be PRIu64

Even with an unsigned size, we can't support more than the maximum off_t
(2^63-1) rather than the full uint64_t range (2^64-1).  Any positive
size prints the same, and if any caller is passing in a negative size,
things are already weird.  But you are correct that matching unsigned to
PRIu64 is nicer, even if we already make blatant assumptions that all
platforms that qemu runs on can interchange signedness in printf without
repercussion.

>>
>>> +                   info->size);
>>>           return -E2BIG;
>>>       }
>>>
>> [...]
>>
> 
> with fixed:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> (if it is not too late ;)

Depends on whether Paolo wants to send a v2 pull request omitting the
NBD stuff (in which case I'll submit a separate NBD pull request), or
whether we just let the pull request happen (in which case I'll submit
followup patches to address your reviews).  It's slightly cleaner to get
it right from the get-go, but followups are better than nothing, so I'm
not too fussed either way.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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