Re: [PATCH 03/12] libxenguest: short-circuit "all-dirty" handling

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Jan Beulich writes ("Re: [PATCH 03/12] libxenguest: short-circuit "all-dirty" 
handling"):
> On 25.06.2021 19:02, Andrew Cooper wrote:
> > Single all-dirty runs are a debugging technique only.  All production
> > cases are live, and you don't want to fail midway through because a
> > late, large, memory allocation failed.
> 
> I'm afraid I don't understand: I don't move _when_ the allocation
> occurs; I only suppress the allocation (altogether) when the allocated
> memory remains unused.

Thanks for this reply, Jan.  I hope it is satisfactory to Andrew; if
not, I hope Andrew will be able to explain.

I am going to give this a provisional

Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>

I may of course withdraw that ack if it is explained to me that this
is patch is wrong.

> >> +        : (void *)-1L;
> > 
> > This is a pointer loaded with a timebomb, which doesn't trigger NULL
> > pointer checks, and for which {set,clear}_bit(dirty_bitmap, large_pfn)
> > won't fault and will instead corrupt random areas of the address space.
> 
> Yeah, this isn't very nice, and gets done away with again in a later
> patch. I'd prefer to keep it like it is (assuming the later change
> will also go in), but if really deemed necessary I can move that code
> re-arrangement here, such that the use of (void *)-1L wouldn't be
> necessary anymore. (You may have noticed that all I did this for is
> to be able to pass the !dirty_bitmap later in the function, and that
> I deliberately only update the local variable, not the hbuf, making
> pretty certain that this pointer isn't going to be de-referenced.)

I agree that this -1L is very unpleasant.

I'm not going to say that you need to restructure your series, but
please make sure you don't commit this patch without the fix.

Thanks,
Ian.