[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Libguestfs] [libnbd PATCH v3 05/22] states: Prepare to receive 64-b
From: |
Laszlo Ersek |
Subject: |
Re: [Libguestfs] [libnbd PATCH v3 05/22] states: Prepare to receive 64-bit replies |
Date: |
Thu, 8 Jun 2023 13:43:43 +0200 |
On 6/7/23 16:55, Richard W.M. Jones wrote:
> On Thu, Jun 01, 2023 at 11:04:05AM +0200, Laszlo Ersek wrote:
>> On 5/25/23 15:00, Eric Blake wrote:
>>> @@ -69,11 +75,18 @@ REPLY.STRUCTURED_REPLY.RECV_REMAINING:
>>> REPLY.STRUCTURED_REPLY.CHECK:
>>> struct command *cmd = h->reply_cmd;
>>> uint16_t flags, type;
>>> - uint32_t length;
>>> + uint64_t length;
>>> + uint64_t offset = -1;
>>
>> (6) I disagree with initializing the local variable "offset" here.
>>
>> Below (in the rest of REPLY.STRUCTURED_REPLY.CHECK), we only read
>> "offset" back if "extended_headers" is set. But if "extended_headers" is
>> set, we also store a value to "offset", before the read.
>>
>> Initializing "offset" to -1 suggests that the code might otherwise read
>> an indeterminate value from "offset" -- but that's not the case.
>
> You may find that the compiler will give a warning. It's usually not
> good about dealing with the case where a variable being initialized +
> used depends on another variable being true.
Good point; that reminds me we used to encounter that issue specifically
in IA32 edk2 builds, but only when using old gcc (RHEL-7 era, gcc-4.8.5).
Of course it might still happen today. If that's the case, can we
comment the code that we initialize "offset" for shutting up the compiler?
Thanks!
Laszlo