[Libguestfs] [PATCH libnbd v3 2/3] copy: Use preferred block size for copying

[Laszlo Ersek]

On 06/30/22 17:47, Richard W.M. Jones wrote:
> On Thu, Jun 30, 2022 at 05:38:14PM +0200, Laszlo Ersek wrote:
>> On 06/29/22 15:36, Richard W.M. Jones wrote:
>>> + * The invariant for '*i' is always an extent which starts before or
>>> + * equal to the current offset.
>>> + */
>>> +static bool
>>> +only_zeroes (const extent_list exts, size_t *i,
>>
>> (5) I think it's a bit wasteful (or at least strange) to pass in the
>> entire vector structure; we certainly don't need the "cap" field. Other
>> functions in libnbd seem to take pointers to vectors (which is what I'd
>> expect).
> 
> Note it's only passing in 3 fields (24 bytes), not the entire array
> (and I guess it should inline the whole function).  Does that change
> things?

It's not wrong by any means, just unusual :) Whole-structure passing and
returning is really rare in my understanding, div() is one standard
function that does it. I don't insist :)

> 
>>> @@ -177,7 +229,10 @@ worker_thread (void *wp)
>>>    extent_list exts = empty_vector;
>>>  
>>>    while (get_next_offset (&offset, &count)) {
>>> +    struct command *command;
>>>      size_t i;
>>> +    bool is_zeroing = false;
>>> +    uint64_t zeroing_start = 0;
>>
>> (14) I think we should not initialize "zeroing_start". It suggests that
>> the initializer value is meaningful, but it is not.
> 
> GCC warns if you don't initialize it!  It doesn't understand that
> is_zeroing is related to the validity of zeroing_start.

We use "#pragma GCC diagnostic" elsewhere in libnbd (in particular in
"copy/progress.c"), can we use it here too? (Just for this one variable?)

If not, I'd suggest appending a small comment:

    uint64_t zeroing_start = 0; /* shut up invalid GCC warning */

> Thanks for the other suggestions, I'll rework things.
> 
> Rich.
> 

Thanks!
Laszlo