From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Egger <Christoph.Egger@amd.com>
Subject: Re: [PATCH] libxl: blktap2 portiblity fixes
Date: Mon, 26 Jul 2010 18:05:06 +0200
Message-ID: <201007261805.06746.Christoph.Egger@amd.com>
References: <201007201831.21416.Christoph.Egger@amd.com>
	<201007261256.37110.Christoph.Egger@amd.com>
	<19533.41294.111030.339525@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <19533.41294.111030.339525@mariner.uk.xensource.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Monday 26 July 2010 16:53:02 Ian Jackson wrote:
> 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.

No, c/s 21834 has the hash e76befc7fe2d.

> 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.

That's doable but I don't see if a standalone const-correctness
patch makes sense just to make gcc happy with an other patch.

> > 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.

Should this header be re-used by tools/console/daemon/io.c ?
If yes, where is the best place to put osdep.h ?

>
> > -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.

hg record is your friend.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632