Re: [Xen-devel] [PATCH v3 18/28] tools/libxl: Infrastructure to convert a legacy stream

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("[PATCH v3 18/28] tools/libxl: Infrastructure to convert 
a legacy stream"):
> Provide a thin wrapper around exec()ing the python conversion utility,
> and a stub implementation for cases where conversion is not wanted
> (i.e. not x86).
> 
> One complication is that the caller of this interface needs to assume
> ownership of the output fd, to prevent it being closed while still in
> use in a datacopier.
...

Thanks.

I have just very minor comments:

> +int libxl__convert_legacy_stream(libxl__egc *egc,
> +                                 libxl__conversion_helper_state *chs)
> +{
...
> +            "--width",
> +#ifdef __i386__
> +            "32",
> +#else
> +            "64",
> +#endif

Firstly, and sorry to not spot this the last time: can we get rid of
this ifdeffery ?

I'm afraid I don't understand why this script needs to be told the
toolstack build bit width.  (If indeed the toolstack build bit width
is the right thing, can't the script tell what bitness it is running
on, some other way?)

If we do need to pass in the bitness then there must be some other way
to find it other than an adhoc #ifdef.

I appreciate that this seems very pernickety for compatibility code
which will only be executed on i386 or amd64, but #ifdefs like this
make the code hard to read IMO.


And some style complaints:

> +struct libxl__conversion_helper_state {
...
> +    void (*completion_callback)(
> +        libxl__egc *egc, libxl__conversion_helper_state *chs, int rc);

Please format this like this:

> +_hidden void libxl__conversion_helper_init(libxl__conversion_helper_state\
 *chs);
> +_hidden int libxl__convert_legacy_stream(libxl__egc *egc,
> +                                         libxl__conversion_helper_state *\
chs);
> +_hidden void libxl__conversion_helper_abort(libxl__egc *egc,
> +                                            libxl__conversion_helper_state\
 *chs,
> +                                            int rc);

Please rewrap these like this:

  +_hidden void libxl__conversion_helper_init
  +                    (libxl__conversion_helper_state *chs);
  +_hidden int libxl__convert_legacy_stream(libxl__egc *egc,
  +                    libxl__conversion_helper_state *chs);
  +_hidden void libxl__conversion_helper_abort(libxl__egc *egc,
  +                    libxl__conversion_helper_state *chs, int rc);

or something similar.


Thanks,
Ian.

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