Xen devel - Jul 2010 - [PATCH] libxl: blktap2 portiblity fixes

Hi!

Attached patch moves blktap2 (which is Linux specific) into
a linux specific file.
Combined with the previous libxl patch, libxl builds again on NetBSD.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

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


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

On Tue, 20 Jul 2010, Christoph Egger wrote:
> 
> Hi!
> 
> Attached patch moves blktap2 (which is Linux specific) into
> a linux specific file.
> Combined with the previous libxl patch, libxl builds again on NetBSD.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 

this patch breaks the build:

libxenlight.so: undefined reference to `libxl_blktap_enabled''
libxenlight.so: undefined reference to `libxl_blktap_devpath''
collect2: ld returned 1 exit status


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

On Tuesday 20 July 2010 18:46:30 Stefano Stabellini
wrote:
> On Tue, 20 Jul 2010, Christoph Egger wrote:
> > Hi!
> >
> > Attached patch moves blktap2 (which is Linux specific) into
> > a linux specific file.
> > Combined with the previous libxl patch, libxl builds again on NetBSD.
> >
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
>
> this patch breaks the build:
>
> libxenlight.so: undefined reference to `libxl_blktap_enabled''
> libxenlight.so: undefined reference to `libxl_blktap_devpath''
> collect2: ld returned 1 exit status
Oh, I see the problem. The wrong lines are those:

LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
+LIBXL_OBJS-$(CONFIG_Linux) = libxl_linux.o
+LIBXL_OBJS-$(CONFIG_NetBSD) = libxl_netbsd.o

New corrected version is attached. It also reverts
c/s 21835 as this showed up due to this bug in my tree.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

On Wed, 21 Jul 2010, Christoph Egger wrote:
> On Tuesday 20 July 2010 18:46:30 Stefano Stabellini wrote:
> > On Tue, 20 Jul 2010, Christoph Egger wrote:
> > > Hi!
> > >
> > > Attached patch moves blktap2 (which is Linux specific) into
> > > a linux specific file.
> > > Combined with the previous libxl patch, libxl builds again on
NetBSD.
> > >
> > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> >
> > this patch breaks the build:
> >
> > libxenlight.so: undefined reference to `libxl_blktap_enabled''
> > libxenlight.so: undefined reference to `libxl_blktap_devpath''
> > collect2: ld returned 1 exit status
> 
> Oh, I see the problem. The wrong lines are those:
> 
> LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
> +LIBXL_OBJS-$(CONFIG_Linux) = libxl_linux.o
> +LIBXL_OBJS-$(CONFIG_NetBSD) = libxl_netbsd.o
> 
> New corrected version is attached. It also reverts
> c/s 21835 as this showed up due to this bug in my tree.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
This seems to be the right fix for the compiling issue.
Acked by me.


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

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity
fixes"):
> New corrected version is attached. It also reverts
> c/s 21835 as this showed up due to this bug in my tree.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
I applied this, but I found that it broke the build.  I have reverted
these three patches of yours:

  df9d8319bd37 Fix blktap2 NetBSD build and also revert broken change
  e76befc7fe2d portability fixes from tools/console
  24277e3237ca Fix linking error when creating the xl binary.

Please come back with a proper fix that doesn''t break the build.  Then
I''ll test and apply it.

Ian.

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

On Friday 23 July 2010 20:05:50 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2
portiblity
fixes"):
> > New corrected version is attached. It also reverts
> > c/s 21835 as this showed up due to this bug in my tree.
> >
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
>
> I applied this, but I found that it broke the build.  I have reverted
> these three patches of yours:
>
>   df9d8319bd37 Fix blktap2 NetBSD build and also revert broken change
>   e76befc7fe2d portability fixes from tools/console
>   24277e3237ca Fix linking error when creating the xl binary.
Can you use the c/s numbers, please?

It was not necessary to backout c/s 21834 as this wasn''t the root
cause.
> Please come back with a proper fix.
Attached.

Re-include c/s 21834 which takes over portability fixes from tools/console
to make libxl_bootloader.c build on NetBSD. Also use $(UTIL_LIBS) in the
Makefile as done in tools/console/Makefile.

blktapctl is build on Linux only. Introduce two API functions
(libxl_blktap_*) which I have discussed with Stefano
in end of June/beginning of July.
Implement the Linux API by moving the
Linux specific code from libxl.c into libxl_linux.c.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> that doesn''t break the build 
   ... on Linux you mean ?
> Then I''ll test and apply it.
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


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

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@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2010-07-26 at 15:53 +0100, Ian Jackson wrote:
>   * Code motion between files, and creation of libxl_linux.c;
>   * Provide libxl_netbsd.c and associated Makefile changes to
>     disable blktap2 on netbsd 
I wonder if this might be better as libxl_blk_blktap.c and
libxl_blk_none.c or something to correspond to the presence of the
specific feature rather than an OS which happens to implement the
feature? (I guess in this specific instance the chances of blktap
cropping up on another OS is pretty slim).

Ian.


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

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity
fixes"):
> On Mon, 2010-07-26 at 15:53 +0100, Ian Jackson wrote:
> >   * Code motion between files, and creation of libxl_linux.c;
> >   * Provide libxl_netbsd.c and associated Makefile changes to
> >     disable blktap2 on netbsd 
> 
> I wonder if this might be better as libxl_blk_blktap.c and
> libxl_blk_none.c or something to correspond to the presence of the
> specific feature rather than an OS which happens to implement the
> feature?
Good point.
> (I guess in this specific instance the chances of blktap
> cropping up on another OS is pretty slim).
Well, but there''s no harm to naming the feature rather than the
platform.  It''s good practice.

Ian.


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

On Monday 26 July 2010 17:12:34 Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: blktap2
portiblity
fixes"):
> > On Mon, 2010-07-26 at 15:53 +0100, Ian Jackson wrote:
> > >   * Code motion between files, and creation of libxl_linux.c;
> > >   * Provide libxl_netbsd.c and associated Makefile changes to
> > >     disable blktap2 on netbsd
> >
> > I wonder if this might be better as libxl_blk_blktap.c and
> > libxl_blk_none.c or something to correspond to the presence of the
> > specific feature rather than an OS which happens to implement the
> > feature?
>
> Good point.
>
> > (I guess in this specific instance the chances of blktap
> > cropping up on another OS is pretty slim).
>
> Well, but there''s no harm to naming the feature rather than the
> platform.  It''s good practice.
It''s bad practise to have OS specific code in common code.
There''s a lot of sysfs code in libxl.c - currently only used for
pci passthrough.

In June/July I discussed three other API functions with Stephano
(which are not part of this series):

- libxl_pciback_flr
     performs FLR on the device specified by BFD

- libxl_pciback_bind
     unhooks pci device from dom0 and bind to pciback

- libxl_pciback_unbind
     unbind pci device from pciback and give back to dom0

These should allow to move all sysfs code in libxl.c into a linux specific
file.


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


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

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


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

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2 portiblity
fixes"):
> 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.
Thus proving my point.  In my tree 21834 is 24277e3237ca :-).
I''m sorry that I may have reverted more than was necessary.  I just
wanted to get everything back to a known good state.
> 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.
The reason is that const-correctness changes shouldn''t be checked in
as "portability to netbsd".  It also means that if for any reason
there is some argument or rework needed for other parts of your
patchset, it''s possible to apply the ones which are clearly right.
And it simplifies the review if you can explain for each patch what
one thing it does.
> > 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 ?
Perhaps.  That would make it a public header rather than a private
one.  On the other hand xenconsoled is not a libxl caller so it''s not
clear where that public header would live.  I don''t think that we need
to cross this bridge right away.  Regardless, finding an example
elsewhere in the tree, in directory which doesn''t have an osdep.h,
isn''t a good reason not to put #ifdef #include stuff in the header we
do have for it in libxl.

Ian.

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

On Monday 26 July 2010 18:36:56 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: blktap2
portiblity
fixes"):
> > 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.
>
> Thus proving my point.  In my tree 21834 is 24277e3237ca :-).
I am not refering the c/s to my local tree. I''m refering the c/s to
http://xenbits.xensource.com/staging/xen-unstable.hg/

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


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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



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


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

patch 1: 
Take over portability fixes from tools/console:
include proper headers for openpty(3) and use $(UTIL_LIBS)
include termios.h for tcgetattr & co.

patch 2:
add generic libxl_blktap interface

patch 3:
device_physdisk_major_minor does not intend to modify physpath.
Also needed to make gcc happy with linux blktap implemenation
(required to make patch 5 compile).

patch 4:
blktapctl is only build on Linux. Disable use on non-Linux.

patch 5:
add bltap implementations for linux.
netbsd implementation is under development so just
add a stub for now to allow build.

patch 6:
Make use of libxl_blktap interface introduced in patch 2.
This removes code from libxl.c which uses blktapctl and
therefore broke build on non-Linux.


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


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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



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


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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



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


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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

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


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

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

Christoph Egger writes ("[Xen-devel] [PATCH 0/6] libxl: portiblity
fixes"):
> patch 1: 
> Take over portability fixes from tools/console:
> include proper headers for openpty(3) and use $(UTIL_LIBS)
> include termios.h for tcgetattr & co.
Thanks, this is better.  It is more usual to submit the commit message
for each patch in the same email as each patch.  I''ll reply to each
patch separately.

Ian.

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

Christoph Egger writes ("[Xen-devel] [PATCH 1/6] libxl: portiblity
fixes"):
>  #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 in libxl_osdeps.h as previously discussed.

Ian.

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

Christoph Egger writes ("[Xen-devel] [PATCH 2/6] libxl: portiblity
fixes"):
> --- a/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:48:38 2010 +0200
> +++ b/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:53:12 2010 +0200
> @@ -23,6 +23,8 @@
>  
>  #define _GNU_SOURCE
>  
> +#include <libxl_internal.h>
> +
This is wrong.  libxl_osdeps.h should not include libxl_internal.h.
> +/* libxl_blktap_enabled:
> + *    return true if blktap/blktap2 support is available.
> + */
> +int libxl_blktap_enabled(struct libxl_ctx *ctx);
This is not what libxl_osdeps.h is for.  These kind of functions can
be declared in a new section in libxl_internal.h.

Also, you should divide your patches conceptually, rather than
according to which files they touch.

This patch is wrong because it introduces a couple of function
declarations but it does not introduce the definitions; your later
patch which introduces the definitions is wrong because it introduces
some functions which are intended to replace existing code, but the
patch does not replace the existing code and the new functions are not
called anywhere in that patch.

Ian.

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

Christoph Egger writes ("[Xen-devel] [PATCH 4/6] libxl: portiblity
fixes"):
> +ifeq ($(CONFIG_Linux),y)
>  CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control -I$(XEN_BLKTAP2)/include
$(CFLAGS_include)
>  LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control -lblktapctl
> +else
> +CFLAGS_libblktapctl > +LDFLAGS_libblktapctl > +endif
I think this should be in the same patch as provides the stub
implementation of the blktap functions for non-blktap systems.

Ian.

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

Christoph Egger writes ("[Xen-devel] [PATCH 5/6] libxl: portiblity
fixes"):
>  LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
> +LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
> +LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o
As previously discussed, these files should probably be called
libxl_blktap2.c and libxl_noblktap2.c or some such.
> +const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
> +                                 const char *disk,
> +                                 libxl_disk_phystype phystype)
You seem to be moving code about by adding it in one patch and
deleting it in another; as I mentioned before, that''s not the right
thing to do.

When submitting a patch series, every individual patch should make
sense by itself and leave a sane and compilable tree.

Ian.

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

On Tuesday 27 July 2010 18:34:21 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 2/6] libxl: portiblity
fixes"):
> > --- a/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:48:38 2010 +0200
> > +++ b/tools/libxl/libxl_osdeps.h	Tue Jul 27 13:53:12 2010 +0200
> > @@ -23,6 +23,8 @@
> >
> >  #define _GNU_SOURCE
> >
> > +#include <libxl_internal.h>
> > +
>
> This is wrong.  libxl_osdeps.h should not include libxl_internal.h.
>
> > +/* libxl_blktap_enabled:
> > + *    return true if blktap/blktap2 support is available.
> > + */
> > +int libxl_blktap_enabled(struct libxl_ctx *ctx);
>
> This is not what libxl_osdeps.h is for.  These kind of functions can
> be declared in a new section in libxl_internal.h.
>
> Also, you should divide your patches conceptually, rather than
> according to which files they touch.
>
> This patch is wrong because it introduces a couple of function
> declarations but it does not introduce the definitions; your later
> patch which introduces the definitions is wrong because it introduces
> some functions which are intended to replace existing code, but the
> patch does not replace the existing code and the new functions are not
> called anywhere in that patch.
The function declarations are the API and the function defintions
are the OS dependent implementations of the API.
Implementations and use of the API is used in different patches.
This is my understanding of defining and implementing an API
in C.
blktap support for linux and netbsd are very different in their 
implementation.
In netbsd, blktap will be implemented using puffs
(http://netbsd.gw.com/cgi-bin/man-cgi?puffs+3+NetBSD-current)

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


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

On Tuesday 27 July 2010 19:00:07 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 5/6] libxl: portiblity
fixes"):
> >  LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o
> > +LIBXL_OBJS-$(CONFIG_Linux) += libxl_linux.o
> > +LIBXL_OBJS-$(CONFIG_NetBSD) += libxl_netbsd.o
>
> As previously discussed, these files should probably be called
> libxl_blktap2.c and libxl_noblktap2.c or some such.
Discussed between you and Ian Campell. Both of you believe
that blktap support on NetBSD will never exist, right ?

blktap support for NetBSD is under development and will be
very different in its implementation.
> > +const char *libxl_blktap_devpath(struct libxl_ctx *ctx,
> > +                                 const char *disk,
> > +                                 libxl_disk_phystype phystype)
>
> You seem to be moving code about by adding it in one patch and
> deleting it in another;
In this case I call this ''factor out OS dependent code''.

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


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

On 28/07/2010 10:06, "Christoph Egger" <Christoph.Egger@amd.com>
wrote:
>> This patch is wrong because it introduces a couple of function
>> declarations but it does not introduce the definitions; your later
>> patch which introduces the definitions is wrong because it introduces
>> some functions which are intended to replace existing code, but the
>> patch does not replace the existing code and the new functions are not
>> called anywhere in that patch.
> 
> The function declarations are the API and the function defintions
> are the OS dependent implementations of the API.
> Implementations and use of the API is used in different patches.
> This is my understanding of defining and implementing an API
> in C.
I find that kind of way of splitting up a patch series annoying as well. As
Ian said, we want each patch to be a logical and separate whole. That means
providing an interface *and* its implementation. Possibly its users as well,
depending on how complicated that bit is -- it''s certainly arguable
they
belong in a separate patch, at least.
> blktap support for linux and netbsd are very different in their
> implementation.
> In netbsd, blktap will be implemented using puffs
> (http://netbsd.gw.com/cgi-bin/man-cgi?puffs+3+NetBSD-current)
A bit of a sidestep I know, but: shouldn''t the blktap library be hiding
this
osdep stuff?

 -- Keir



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

On Wednesday 28 July 2010 11:21:54 Keir Fraser wrote:
> On 28/07/2010 10:06, "Christoph Egger"
<Christoph.Egger@amd.com> wrote:
> >> This patch is wrong because it introduces a couple of function
> >> declarations but it does not introduce the definitions; your later
> >> patch which introduces the definitions is wrong because it
introduces
> >> some functions which are intended to replace existing code, but
the
> >> patch does not replace the existing code and the new functions are
not
> >> called anywhere in that patch.
> >
> > The function declarations are the API and the function defintions
> > are the OS dependent implementations of the API.
> > Implementations and use of the API is used in different patches.
> > This is my understanding of defining and implementing an API
> > in C.
>
> I find that kind of way of splitting up a patch series annoying as well. As
> Ian said, we want each patch to be a logical and separate whole. That means
> providing an interface *and* its implementation. Possibly its users as
> well, depending on how complicated that bit is -- it''s certainly
arguable
> they belong in a separate patch, at least.
>
> > blktap support for linux and netbsd are very different in their
> > implementation.
> > In netbsd, blktap will be implemented using puffs
> > (http://netbsd.gw.com/cgi-bin/man-cgi?puffs+3+NetBSD-current)
>
> A bit of a sidestep I know, but: shouldn''t the blktap library be
hiding
> this osdep stuff?
That''s a good point. With this in mind, having libxl_blktap.c and 
libxl_noblktap.c as the Ian''s suggested makes perfect sense to me.

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


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

On Tuesday 27 July 2010 18:31:24 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 1/6] libxl: portiblity
fixes"):
> >  #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 in libxl_osdeps.h as previously discussed.
Done.

Take over portability fixes from tools/console:
include proper headers for openpty(3) and use $(UTIL_LIBS)
include termios.h for tcgetattr & co.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

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


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

Move blktap specific code into libxl_blktap.c

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

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


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

Hi!

Attached patch fixes this compile error:

xl_cmdimpl.c: In function ''create_domain'':
xl_cmdimpl.c:1099: warning: ''action'' may be used uninitialized
in this
function

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

On Tuesday 27 July 2010 18:58:26 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH 4/6] libxl: portiblity
fixes"):
> > +ifeq ($(CONFIG_Linux),y)
> >  CFLAGS_libblktapctl = -I$(XEN_BLKTAP2)/control
-I$(XEN_BLKTAP2)/include
> > $(CFLAGS_include) LDFLAGS_libblktapctl = -L$(XEN_BLKTAP2)/control
> > -lblktapctl
> > +else
> > +CFLAGS_libblktapctl > > +LDFLAGS_libblktapctl > > +endif
>
> I think this should be in the same patch as provides the stub
> implementation of the blktap functions for non-blktap systems.
Done.

Attached patch adds the stub implementations for
non-blktap systems.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

On Wed, 2010-07-28 at 12:42 +0100, Christoph Egger
wrote:
> Hi!
> 
> Attached patch fixes this compile error:
> 
> xl_cmdimpl.c: In function ''create_domain'':
> xl_cmdimpl.c:1099: warning: ''action'' may be used
uninitialized in this
> function
Thanks.

This can''t actually happen in practice today since the switch statement
covers all of the possible values of info->shutdown_reason. (in a
previous version of the series which introduced this code
shutdown_reason was an enum so the compiler knew this).

However to be robust it is probably worth adding a default: case to the
switch and logging the unknown shutdown code.

Ian.


Subject: xl: log unknown domain shutdown reason and default to destroy

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 479d042f25e4 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Jul 28 12:07:44 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Wed Jul 28 13:21:16 2010 +0100
@@ -1113,6 +1113,9 @@ static int handle_domain_death(libxl_ctx
     case SHUTDOWN_watchdog:
         action = d_config->on_watchdog;
         break;
+    default:
+        LOG("Unknown shutdown reason code %s. Destroying domain.",
info->shutdown_reason);
+        action = ACTION_DESTROY;
     }
 
     LOG("Action for shutdown reason code %d is %s",
info->shutdown_reason, action_on_shutdown_names[action]);

> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> 


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

On Wednesday 28 July 2010 14:22:37 Ian Campbell wrote:
> On Wed, 2010-07-28 at 12:42 +0100, Christoph Egger wrote:
> > Hi!
> >
> > Attached patch fixes this compile error:
> >
> > xl_cmdimpl.c: In function ''create_domain'':
> > xl_cmdimpl.c:1099: warning: ''action'' may be used
uninitialized in this
> > function
>
> Thanks.
>
> This can''t actually happen in practice today since the switch
statement
> covers all of the possible values of info->shutdown_reason. (in a
> previous version of the series which introduced this code
> shutdown_reason was an enum so the compiler knew this).
>
> However to be robust it is probably worth adding a default: case to the
> switch and logging the unknown shutdown code.
>
> Ian.
>
>
> Subject: xl: log unknown domain shutdown reason and default to destroy
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff -r 479d042f25e4 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed Jul 28 12:07:44 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Wed Jul 28 13:21:16 2010 +0100
> @@ -1113,6 +1113,9 @@ static int handle_domain_death(libxl_ctx
>      case SHUTDOWN_watchdog:
>          action = d_config->on_watchdog;
>          break;
> +    default:
> +        LOG("Unknown shutdown reason code %s. Destroying
domain.",
> info->shutdown_reason); +        action = ACTION_DESTROY;
>      }
>
>      LOG("Action for shutdown reason code %d is %s",
info->shutdown_reason,
> action_on_shutdown_names[action]);
>
Acked-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap specific
code into libxl_blktap.c"):
> Move blktap specific code into libxl_blktap.c
Thanks, this is going in the right direction.  But can you please
split up the moving code into a different file, from the changes to
that code ?

As it is it is almost impossible to see what changes you have made to
the code you are moving, as we have

 diff old-file
   stuff
 - old
 - code
   more stuff
 diff new-file
 + newly
 + reorganised
 + code


If the code needs to be reorganised so that it can be moved, you
should do this in two patches, so we end up with:

 [PATCH 1/2] reorganise preparatory to moving

 diff old-file
   stuff
 - old
 + newly
 + reorganised
   code
   more stuff

 [PATCH 2/2] move blktap-specific code to libxl_blktap.c
 Purely moving code about, no changes.

 diff old-file
   stuff
 - newly
 - reorganised
 - code
   old stuff
 diff new-file
 + newly
 + reorganised
 + code


Ian.

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

On Thu, 29 Jul 2010, Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap
specific code into libxl_blktap.c"):
> > Move blktap specific code into libxl_blktap.c
> 
> Thanks, this is going in the right direction.  But can you please
> split up the moving code into a different file, from the changes to
> that code ?
> 
> As it is it is almost impossible to see what changes you have made to
> the code you are moving, as we have
> 
>  diff old-file
>    stuff
>  - old
>  - code
>    more stuff
>  diff new-file
>  + newly
>  + reorganised
>  + code
> 
> 
> If the code needs to be reorganised so that it can be moved, you
> should do this in two patches, so we end up with:
> 
>  [PATCH 1/2] reorganise preparatory to moving
> 
>  diff old-file
>    stuff
>  - old
>  + newly
>  + reorganised
>    code
>    more stuff
> 
>  [PATCH 2/2] move blktap-specific code to libxl_blktap.c
>  Purely moving code about, no changes.
> 
>  diff old-file
>    stuff
>  - newly
>  - reorganised
>  - code
>    old stuff
>  diff new-file
>  + newly
>  + reorganised
>  + code
> 
 
I take this chance to say that I greatly prefer inline patches to
attachments.

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

On Thursday 29 July 2010 17:02:25 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH] libxl: move blktap
specific
code into libxl_blktap.c"):
> > Move blktap specific code into libxl_blktap.c
>
> Thanks, this is going in the right direction.  But can you please
> split up the moving code into a different file, from the changes to
> that code ?
>
> As it is it is almost impossible to see what changes you have made to
> the code you are moving, as we have
Is it enough when I explain the change and adjust the commit message
accordingly ?

The libxl_blktap_devpath implementation merges
get_blktap2_devices() and make_blktap2_devices().
>  diff old-file
>    stuff
>  - old
>  - code
>    more stuff
>  diff new-file
>  + newly
>  + reorganised
>  + code
>
>
> If the code needs to be reorganised so that it can be moved, you
> should do this in two patches, so we end up with:
>
>  [PATCH 1/2] reorganise preparatory to moving
>
>  diff old-file
>    stuff
>  - old
>  + newly
>  + reorganised
>    code
>    more stuff
>
>  [PATCH 2/2] move blktap-specific code to libxl_blktap.c
>  Purely moving code about, no changes.
>
>  diff old-file
>    stuff
>  - newly
>  - reorganised
>  - code
>    old stuff
>  diff new-file
>  + newly
>  + reorganised
>  + code
hmm... doing it vice versa is easier to do because the new code
merges to functions (see above).

First patch contains the move and the second patch contains
the merge.


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


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

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: move blktap
specific code into libxl_blktap.c"):
> Is it enough when I explain the change and adjust the commit message
> accordingly ?
No, because we want to be able to review the changes, and people who
look at the repo history in the future will want to be able to see
what was done and why.

Ian.

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

Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxl: move blktap
specific code into libxl_blktap.c"):
> On Thursday 29 July 2010 17:02:25 Ian Jackson wrote:
> >  [PATCH 1/2] reorganise preparatory to moving
...
> >  [PATCH 2/2] move blktap-specific code to libxl_blktap.c
> >  Purely moving code about, no changes.
...
> hmm... doing it vice versa is easier to do because the new code
> merges to functions (see above).
Sorry, I didn''t see that comment (because you didn''t trim the
quoted
text (-:.)
> First patch contains the move and the second patch contains
> the merge.
That would be fine.  The point is not to mix moving code about with
anything else.

Ian.

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

libxl: move blktap-specific code into libxl_blktap.c

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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


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

Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into
libxl_blktap.c"):
> libxl: move blktap-specific code into libxl_blktap.c
Except, once again, this is not _pure_ motion.  You need to split the
patch up so that, in the code motion patch,

  diff -u <(hg diff tools/libxl/libxl.c | perl -pe ''s/^[-+]/ $&
eq "-" ? "+" : "-" /e'' ) <(hg diff
tools/libxl/libxl_blktap.c)

doesn''t produce any substantive output.

Ian.

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

On Thursday 29 July 2010 20:02:09 Ian Jackson wrote:
> Christoph Egger writes ("[PATCH] libxl: move blktap-specific code into
libxl_blktap.c"):
> > libxl: move blktap-specific code into libxl_blktap.c
>
> Except, once again, this is not _pure_ motion.  You need to split the
> patch up so that, in the code motion patch,
>
>   diff -u <(hg diff tools/libxl/libxl.c | perl -pe ''s/^[-+]/
$& eq "-" ?
> "+" : "-" /e'' ) <(hg diff
tools/libxl/libxl_blktap.c)
>
> doesn''t produce any substantive output.
I''m sorry, I don''t know perl. And this command fails with
syntax error: ''(''
unexpected.

I haven''t send the patch yesterday. It was late for me.
You seem to respond always in the late afternoon (CEST).
In which timezone are you ?

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


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

On Friday 30 July 2010 10:40:27 Christoph Egger wrote:
> On Thursday 29 July 2010 20:02:09 Ian Jackson wrote:
> > Christoph Egger writes ("[PATCH] libxl: move blktap-specific code
into
>
> libxl_blktap.c"):
> > > libxl: move blktap-specific code into libxl_blktap.c
> >
> > Except, once again, this is not _pure_ motion.  You need to split the
> > patch up so that, in the code motion patch,
> >
> >   diff -u <(hg diff tools/libxl/libxl.c | perl -pe
''s/^[-+]/ $& eq "-" ?
> > "+" : "-" /e'' ) <(hg diff
tools/libxl/libxl_blktap.c)
> >
> > doesn''t produce any substantive output.
>
> I''m sorry, I don''t know perl. And this command fails with
syntax error: ''(''
> unexpected.
>
> I haven''t send the patch yesterday.
Wanted to say: I haven''t send the patch with the change yesterday.
> It was late for me. 
> You seem to respond always in the late afternoon (CEST).
> In which timezone are you ?
>
> 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


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