Re: [Xen-devel] [PATCH] xenguest: Add xsa-25 decompression limit prototypes

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-01-28 at 10:25 +0000, Andrew Cooper wrote:
> On 28/01/13 09:25, Ian Campbell wrote:
> > On Fri, 2013-01-25 at 18:04 +0000, Andrew Cooper wrote:
> >> To allow xenguest consumers to also make use of the extra protection added 
> >> as
> >> a result of xsa-25.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>
> >> diff -r 5af4f2ab06f3 -r daec50a41570 tools/libxc/xenguest.h
> >> --- a/tools/libxc/xenguest.h
> >> +++ b/tools/libxc/xenguest.h
> >> @@ -177,6 +177,13 @@ int xc_dom_linux_build(xc_interface *xch
> >>                   unsigned int console_evtchn,
> >>                   unsigned long *console_mfn);
> >>  
> >> +#ifndef XC_HAVE_DECOMPRESS_LIMITS
> >> +#define XC_HAVE_DECOMPRESS_LIMITS
> >> +#endif
> > This sort of thing isn't in general needed for the libxc interface,
> > which already makes very little in the way of API stability guarantees.
> > The only existing example uses XENCTRL_HAS_* so if this is really felt
> > to be useful it should match.
> >
> > Also this file has multiple inclusion guards so the ifndef really isn't
> > necessary.
> >
> >> +int xc_dom_kernel_max_size(struct xc_dom_image *dom, size_t sz);
> >> +int xc_dom_ramdisk_max_size(struct xc_dom_image *dom, size_t sz);
> > These duplicate the ones in xc_dom.h, if the prototypes there aren't
> > sufficient then they should be moved not repeated.
> 
> xenguest.h includes no files whatsoever,

I said you should move them, not change the #includes. Not that changing
the #includes isn't a good idea too.

>  and every single prototype in
> it is a duplicate of other functions in xc_*.h header files.

I don't know what tree you are looking at but that's not the case in
unstable. A random sampling of xc_linux_build, xc_linux_build_mem and
xc_await_suspend shows that they are only defined in xenguest.h.
Likewise none of the functions in xc_dom.h that I checked were defined
anywhere else but in that file.

>   This
> appears to be the point.  Furthermore, it is the only file referenced by
> the ocaml subs for libxc, 

What on earth are you talking about:
$ rgrep \#include tools/ocaml/libs/xc/
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <stdlib.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <errno.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <caml/alloc.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <caml/memory.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <caml/signals.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <caml/fail.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <caml/callback.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <sys/mman.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <stdint.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <string.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include <xenctrl.h>
tools/ocaml/libs/xc/xenctrl_stubs.c:#include "mmap_stubs.h"

Not only is it not the only file referenced, it's not even referenced at
all!

> and the xenguest helper utility for Xapi.

Possibly, but that doesn't preclude fixing that if that is the right
thing to do.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel