Re: [PATCH] libxc: remove most of tools/libxc/xc_dom_compat_linux.c

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-10-23 at 11:53 +0200, Juergen Gross wrote:
> On 10/23/2015 11:42 AM, Ian Campbell wrote:
> > On Fri, 2015-10-23 at 09:15 +0200, Juergen Gross wrote:
> > > On 10/22/2015 05:38 PM, Ian Campbell wrote:
> > > > On Thu, 2015-10-22 at 16:22 +0100, Ian Jackson wrote:
> > > > > Juergen Gross writes ("Re: [Xen-devel] [PATCH] libxc: remove most
> > > > > of
> > > > > tools/libxc/xc_dom_compat_linux.c"):
> > > > > > On 10/06/2015 03:17 PM, Ian Campbell wrote:
> > > > > > > xc_dom_linux_build is implemented in terms of the non-compat
> > > > > > > xc_dom_*
> > > > > > > functions, so it should be possible to do what you want with
> > > > > > > out
> > > > > > > using the
> > > > > > > compat wrapper.
> > > > > > > 
> > > > > > > If there is some obscure reason this isn't the case then we
> > > > > > > should
> > > > > > > fix
> > > > > > > that, not carry around the compat options for ever as a
> > > > > > > workaround
> > > > > > > (fixes
> > > > > > > include but are not limited to promoting xc_dom_linux_build
> > > > > > > into
> > > > > > > a
> > > > > > > non
> > > > > > > -compat helper).
> > > > > 
> > > > > I agree with this approach.
> > > > > 
> > > > > > Any further comments?
> > > > > > 
> > > > > > Andrew, are you okay with Ian's statement?
> > > > > > 
> > > > > > Ian, does this mean you are Ack-ing the patch?
> > > > > 
> > > > > Accordingly, in the absence of renewed objections, or alternative
> > > > > proposals, the original patch is:
> > > > > 
> > > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > 
> > > > There was a conflict with "libxc: unify xc_dom_p2m_{host/guest}",
> > > > where
> > > > xc_dom_p2m_host became xc_dom_p2m. I tried to resolve in what I
> > > > thought
> > > > was
> > > > the obvious way, but then I got many instances of:
> > > > 
> > > >       In file included from libxl.c:19:0:
> > > >       libxl_internal.h:1612:43: error: 'struct xc_dom_image'
> > > > declared
> > > > inside parameter list [-Werror]
> > > >                                           struct xc_dom_image
> > > > *dom);
> > > >                                                  ^
> > > >       libxl_internal.h:1612:43: error: its scope is only this
> > > > definition
> > > > or declaration, which is probably not what you want [-Werror]
> > > > 
> > > > Not sure if the original patch was wrong, has bit-rotted, or I
> > > > messed
> > > > up
> > > > the conflict resolution. This happens on all arches.
> > > > 
> > > > Actually, looking back at it, the added "struct xc_dom_image" in
> > > > libxl_arch.h is surely wrong, the right answer would be to include
> > > > xc_dom.h
> > > > somewhere appropriate it might be tolerable to just leave it in
> > > > xenguest.h.
> > > > 
> > > > Juergen, please investigate the build failure, fix the above and
> > > > resubmit.
> > > 
> > > That was easy. Just removing the definition for libxl_arch.h, include
> > > xc_dom.h from libxl_internal.h and modify xc_dom.h to tolerate
> > > including
> > > it multiple times.
> > > 
> > > I've stumbled over another issue:
> > > 
> > > I don't know what I did wrong, but obviously the patch was built on
> > > top
> > > of the libxc python wrappers removal patch. Without that there are
> > > still
> > > some functions in use which I wanted to remove in
> > > xc_dom_compat_linux.c
> > > 
> > > As there was no objection for the intention of removing most of the
> > > wrappers I'll resend the xc_dom_compat_linux.c cleanup patch together
> > > with the libxc python wrappers cleanup in a series.
> > 
> > OK. Please put the xc_dom_compat_linux.c parts towards the head of the
> > series, such that they don't get blocked by any subsequent kvetching
> > about
> > any specific Python removal. (Except you should remove the Python
> > wrappers
> > for anything in xc_dom_compat_linux.c in the same patch as the removal
> > of
> > the C version).
> 
> Just to get it right: You are suggesting I do two patches:
> 
> - Patch 1: cleanup of xc_dom_compat_linux.c + removal of all python
>    wrappers affected by this cleanup (this would be xc.linux_build() and
>    xc.getBitSize() ).

Hrm, xc_get_bit_size seems like an odd thing to be in xc_dom_compat_linux.c(i.e. I wasn't expecting that sort of thing to be there).

I'd suggest doing most of xc_dom_compat_linux.c in a first patch (i.e. all
the semantically related "build a domain" type stuff) and then
xc_dom_bit_size separately.

> - Patch 2: removal of the rest of the python wrappers

I'm not sure exactly how much you are planning to remove in one fell swoop
here. If it is lots of seemingly unrelated stuff then my preference would
probably be to do it if possible in some vaguely related chunks by
functionality, such that an objection to one piece of functionality being
removed needn't block the entire patch.

Ian.