[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Jan Beulich writes ("Re: [PATCH 03/12] libxenguest: short-circuit "all-dirty" 
> 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.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.