Re: [Xen-Devel] [PATCH] [GSOC14] refactored mempaging code from xenpaging to libxc.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/06/14 15:21, Ian Campbell wrote:
> On Thu, 2014-06-12 at 15:11 +0100, Andrew Cooper wrote:
>
>
>>>         
>>>         > +    if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB )
>>>         
>>>         
>>>         What is this check for?
>>> foreign_batch has the old semantics of setting the MSB of the pfn in
>>> error. Maybe use map_foreign_bulk with cleaner error reporting? 
>>>
>> Eww.  That is a nasty interface, and will completely break 32bit
>> toolstacks on machines with hotplug regions above the 8TB boundary.
>>
>> Please do use map_foreign_bulk().
> The stated purpose of this patch is code motion. Please don't encourage
> people to also make functional changes in such patches.
>
> If there is to be any cleanups/improvements then they should be done
> later, but that won't affect the acceptance of this patch (assuming it
> really is just motion, I've not checked in detail).
>
> Ian.
>

Refactoring is not necessarily code motion, although in this case it
does appear to be almost exclusively motion.

However, it is motion of code from an example utility into a main
library, and I do not feel it is appropriate to be moving code with bugs
like this into libxc.  (Perhaps I am overly jaded by the amount of
fixing up the migration code needed in areas like this)

Two solutions come to mind: either change this patch to be "implement
xenpaging initialisation in libxenguest" and fix the code up as it goes
in, or fix it up in xenpaging and move it as a second patch.

~Andrew