Re: [PATCH v2 03/13] libxenguest: deal with log-dirty op stats overflow

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.07.2021 17:41, Andrew Cooper wrote:
> On 05/07/2021 16:13, Jan Beulich wrote:
>> In send_memory_live() the precise value the dirty_count struct field
>> gets initialized to doesn't matter much (apart from the triggering of
>> the log message in send_dirty_pages(), see below), but it is important
>> that it not be zero on the first iteration (or else send_dirty_pages()
>> won't get called at all). Saturate the initializer value at the maximum
>> value the field can hold.
> 
> I've already explained why this is broken...
> 
>> While there also initialize struct precopy_stats' respective field to a
>> more sane value: We don't really know how many dirty pages there are at
>> that point.
>>
>> In suspend_and_send_dirty() and verify_frames() the local variables
>> don't need initializing at all, as they're only an output from the
>> hypercall which gets invoked first thing.
>>
>> In send_checkpoint_dirty_pfn_list() the local variable can be dropped
>> altogether: It's optional to xc_logdirty_control() and not used anywhere
>> else.
> 
> ... and why this is broken particularly in the context of a later
> change, and ...
> 
>>
>> Note that in case the clipping actually takes effect, the "Bitmap
>> contained more entries than expected..." log message will trigger. This
>> being just an informational message, I don't think this is overly
>> concerning.
> 
> ... why this isn't ok.
> 
> Why do I bother wasting my time reviewing patches in the first place.

I'm sorry Andrew, but no. I could as well reply "Why do I bother
replying to your review comments?" You did say

"I don't follow.  Migration would be extremely broken if the first
 iteration didn't work correctly, so something else is going on here."

which I replied to:

"As per the title we're talking about overflow situation here. In particular
 the field did end up zero when ctx->save.p2m_size was 0x100000000. I'm not
 claiming in any way that the first iteration would generally not work."

and then nothing else came back from you. I gave it a couple of
days, taking silence as indication that my reply was satisfactory.

Similarly e.g. for you saying

"This is an external interface, and I'm not sure it will tolerate finding
 more than p2m_size allegedly dirty."

with my reply

"But you do realize that a few lines down from here there already was

        policy_stats->dirty_count   = -1;

 ? Or are you trying to tell me that -1 (documented as indicating
 "unknown") is okay on subsequent iterations, but not on the first one?
 That's not being said anywhere ..."

If you expect me to adjust patches rather then just replying verbally,
you won't get around replying back when you get verbal feedback.

Jan