Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity fixes

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity 
fixes"):
> Can you use the c/s numbers, please?

Changeset numbers are not guaranteed to be meaningful outside a
particular tree, particularly in the presence of merges.

> It was not necessary to backout c/s 21834 as this wasn't the root cause.

I think by 21834 you mean 24277e3237ca.  That change was entirely
wrong and you even retracted it yourself!  I see that your patch
doesn't actually reinstate it.

Looking at this patch it's difficult to review and test and there are
some things I would like to see improved.  Can I ask you to try to
split the patch up into separate pieces ?

At the very least, please separate out the following:
  * Adding new interfaces, ie introucing new functions and putting
    code into those functions and replacing it at the original site
    with a call;
  * Const-correctness
  * Code motion between files, and creation of libxl_linux.c;
  * Provide libxl_netbsd.c and associated Makefile changes to
    disable blktap2 on netbsd
  * #include portability fixes for openpty, pty.h etc.

> diff -r 2b768d52bc7f tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c  Sun Jul 25 22:20:47 2010 +0100
> +++ b/tools/libxl/libxl_bootloader.c  Mon Jul 26 12:41:02 2010 +0200
> @@ -15,9 +15,16 @@
>  #include "libxl_osdeps.h"
>  
>  #include <string.h>
> -#include <pty.h>
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include <termios.h>
> +#if defined(__NetBSD__) || defined(__OpenBSD__)
> +#include <util.h>
> +#elif defined(__linux__)
> +#include <pty.h>
> +#elif defined(__sun__)
> +#include <stropts.h>
> +#endif

This should be done by moving the relevant #includes to osdep.h, where
all this kind of thing should be done.

> -int device_physdisk_major_minor(char *physpath, int *major, int *minor)
> +int device_physdisk_major_minor(const char *physpath, int *major, int *minor)

This change should be separated out.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel