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

[Paolo Bonzini]

On 13/07/2017 16:35, Eric Blake wrote:
> 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.

I've fixed the PRIu64 already when applying (and also the tracepoint in
patch 2).

Paolo

signature.asc
Description: OpenPGP digital signature