Xen devel - Mar 2008 - [PATCH][TOOLS] libfsimage: portability fixes

Hi Keir!

Attached patch improves portability and adds fixes for NetBSD to libfsimage.

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


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy


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

Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS] libfsimage: portability
fixes"):
> -SUBDIRS-y += $(shell env CC="$(CC)" ./check-libext2fs)
> +SUBDIRS-y += $(shell $(SHELL) env CC="$(CC)" ./check-libext2fs)
What purpose does this serve ?
> +	/*
> +	 * Make reads from a raw disk sector-aligned. This is a requirement
> +	 * for NetBSD. Split the read up into to three parts to meet this
> +	 * requirement.
> +	 */
Please forgive my ignorance: Does NetBSD offer a different (non-raw)
device which does not have this requirement.  If so perhaps we should
be using it instead - if not, why not ?

Ian.

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

Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS] libfsimage: portability
fixes"):
> -${CC:-gcc} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> +if test -z ${CC}; then CC="gcc"; fi
> +${CC} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> +
This also seems to serve no purpose as far as I can tell.

Ian.

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

Christoph Egger wrote:  [Wed Mar 26 2008, 10:14:33AM
EDT]
> diff -r 966c04d42e94 tools/libfsimage/Makefile
> --- a/tools/libfsimage/Makefile	Wed Mar 26 09:12:57 2008 +0000
> +++ b/tools/libfsimage/Makefile	Wed Mar 26 17:12:55 2008 +0100
> @@ -2,7 +2,7 @@ include $(XEN_ROOT)/tools/Rules.mk
>  include $(XEN_ROOT)/tools/Rules.mk
>  
>  SUBDIRS-y = common ufs reiserfs iso9660 fat
> -SUBDIRS-y += $(shell env CC="$(CC)" ./check-libext2fs)
> +SUBDIRS-y += $(shell $(SHELL) env CC="$(CC)" ./check-libext2fs)
As Ian asked, what was this intended to do?  It''s certainly
wrong:

$ /bin/sh env CC=gcc ./check-libext2fs 
/usr/bin/env: /usr/bin/env: cannot execute binary file
> diff -r 966c04d42e94 tools/libfsimage/check-libext2fs
> --- a/tools/libfsimage/check-libext2fs	Wed Mar 26 09:12:57 2008 +0000
> +++ b/tools/libfsimage/check-libext2fs	Wed Mar 26 17:12:55 2008 +0100
> @@ -1,4 +1,4 @@
> -#!/bin/bash
> +#!/bin/sh
>  
>  cat >ext2-test.c <<EOF
>  #include <ext2fs/ext2fs.h>
> @@ -9,7 +9,9 @@ int main()
>  }
>  EOF
>  
> -${CC:-gcc} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> +if test -z ${CC}; then CC="gcc"; fi
> +${CC} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> +
>  if [ $? = 0 ]; then
>  	echo ext2fs-lib
>  else
This prevents check-libext2fs from being run outside the
Makefile.  It will silently fail compilation if $CC isn''t set.
For Bourne shell and cross-platform compatibility, it should be:

${CC-cc} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1

Aron

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

On Thursday 27 March 2008 11:54:41 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS] libfsimage:
portability
fixes"):
> > -SUBDIRS-y += $(shell env CC="$(CC)" ./check-libext2fs)
> > +SUBDIRS-y += $(shell $(SHELL) env CC="$(CC)"
./check-libext2fs)
>
> What purpose does this serve ?
Everytime when I submitted a patch where I changed /bin/bash to /bin/sh
John Levon came up with a "Build is broken on Solaris" message.
The fix was always the same: Use $(SHELL) as this is explicitely set for
Solaris.
> > +	/*
> > +	 * Make reads from a raw disk sector-aligned. This is a requirement
> > +	 * for NetBSD. Split the read up into to three parts to meet this
> > +	 * requirement.
> > +	 */
>
> Please forgive my ignorance: Does NetBSD offer a different (non-raw)
> device which does not have this requirement.  If so perhaps we should
> be using it instead - if not, why not ?
The raw device pass requests directly to the underlying device, with
only check/adjustments against the partition bounds. Especially it
won''t
try to do read/modify/write for write requests, or expand the read if
it''s
not sector-aligned.

The block device doesn''t have this restriction, but allows only ONE
open,
therefore it is not usable by pygrub. It also has other side-effects
(as it goes through the buffer cache), it''s definitively not useable
for the
NetBSD block device *backend*   or for qemu-dm I/O.

Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy


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

Aron Griffis writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage:
portability fixes"):
> Christoph Egger wrote:  [Wed Mar 26 2008, 10:14:33AM EDT]
> > -${CC:-gcc} -o ext2-test ext2-test.c -lext2fs >/dev/null
2>&1
> > +if test -z ${CC}; then CC="gcc"; fi
> > +${CC} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> 
> This prevents check-libext2fs from being run outside the
> Makefile.  It will silently fail compilation if $CC isn''t set.
> For Bourne shell and cross-platform compatibility, it should be:
${VARIABLE:-defaultvalue} is in SuSv3 (section 2.6.2, `Use Default
Values'') and has been in many previous standards.  Any system whose
/bin/sh can''t cope is hopelessly broken.
> ${CC-cc} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
However, in this case, ${VARIABLE-defaultvalue} is an adequate
substitute so we should use that.  With a default of gcc, since
without gcc you''re doomed anyway.

Ian.

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

On Thursday 27 March 2008 12:05:30 Ian Jackson wrote:
> Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS] libfsimage:
portability
fixes"):
> > -${CC:-gcc} -o ext2-test ext2-test.c -lext2fs >/dev/null
2>&1
> > +if test -z ${CC}; then CC="gcc"; fi
> > +${CC} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> > +
>
> This also seems to serve no purpose as far as I can tell.
This is for the ancient Solaris /bin/sh.  If John Levon confirms that
this and that the $(SHELL)  mentioned in your other mail 
are NOT needed/required to not break Solaris, you can undo these two changes.

Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy


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

Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage:
portability fixes"):
> Everytime when I submitted a patch where I changed /bin/bash to /bin/sh
> John Levon came up with a "Build is broken on Solaris" message.
> The fix was always the same: Use $(SHELL) as this is explicitely set for
> Solaris.
People whose /bin/sh is that badly broken deserve to lose IMO - that
is to say, we shouldn''t inconvenience other people for their benefit.
We''ve had this conversation before.

But in any case your syntax was completely wrong and the script has
#!/bin/sh at the top already so I don''t see why this is relevant to
you.
> On Thursday 27 March 2008 11:54:41 Ian Jackson wrote:
> > Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS] libfsimage:
portability
> > Please forgive my ignorance: Does NetBSD offer a different (non-raw)
> > device which does not have this requirement.  If so perhaps we should
> > be using it instead - if not, why not ?
> 
> The raw device pass requests directly to the underlying device, with
> only check/adjustments against the partition bounds. Especially it
won''t
> try to do read/modify/write for write requests, or expand the read if
it''s
> not sector-aligned.
Yes, but these aren''t desirable in this case.
> The block device doesn''t have this restriction, but allows only
ONE open,
> therefore it is not usable by pygrub. It also has other side-effects
> (as it goes through the buffer cache), it''s definitively not
useable for the
> NetBSD block device *backend*   or for qemu-dm I/O.
The problem is that pygrub wants to open the disk multiple times ?
Or is it that xend has already plumbed in the device to qemu-dm or
what have you, and that prevents a second open ?

There doesn''t seem to me to be any problem with pygrub''s
accesses
going through the buffer cache.

I accept that it''s not suitable for use as the full block backend, but
perhaps the answer is to pass pygrub an edited version of the device
name, or have pygrub edit it itself.  If we were to use the non-raw
device for pygrub and the raw device for qemu-dm, would things work ?

Ian.

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

Hi Keir,

changeset 17309 has one hunk too much:

--- a/tools/libfsimage/check-libext2fs	Thu Mar 27 14:43:20 2008 +0000
+++ b/tools/libfsimage/check-libext2fs	Thu Mar 27 15:13:55 2008 +0000
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 
 cat >ext2-test.c <<EOF
 #include <ext2fs/ext2fs.h>

This causes:

ksh: ./check-libext2fs: No such file or directory

because /bin/bash does not exist on *BSD.
Please apply this:

diff -r ee38b254e98e tools/libfsimage/check-libext2fs
--- a/tools/libfsimage/check-libext2fs  Thu Mar 27 15:13:55 2008 +0000
+++ b/tools/libfsimage/check-libext2fs  Thu Mar 27 18:44:05 2008 +0100
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
 
 cat >ext2-test.c <<EOF
 #include <ext2fs/ext2fs.h>


Christoph


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

Ian Jackson wrote:  [Thu Mar 27 2008, 11:13:41AM EDT]
> ${VARIABLE:-defaultvalue} is in SuSv3 (section 2.6.2, `Use Default
> Values'') and has been in many previous standards.  Any system
whose
> /bin/sh can''t cope is hopelessly broken.
Ah, great.
> > ${CC-cc} -o ext2-test ext2-test.c -lext2fs >/dev/null 2>&1
> 
> However, in this case, ${VARIABLE-defaultvalue} is an adequate
> substitute so we should use that.  With a default of gcc, since
> without gcc you''re doomed anyway.
Okay, thanks.

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

On Thu, Mar 27, 2008 at 03:36:45PM +0000, Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage:
portability fixes"):
> > Everytime when I submitted a patch where I changed /bin/bash to
/bin/sh
> > John Levon came up with a "Build is broken on Solaris"
message.
> > The fix was always the same: Use $(SHELL) as this is explicitely set
for
> > Solaris.
> 
> People whose /bin/sh is that badly broken deserve to lose IMO - that
Putting it mildly, I don''t think that disabling an important platform
because you have a bee in your bonnet about $(SHELL) is a sensible
implementation choice.

john

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

On Thu, Mar 27, 2008 at 04:16:51PM +0100, Christoph Egger wrote:
> On Thursday 27 March 2008 12:05:30 Ian Jackson wrote:
> > Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS] libfsimage:
portability
> fixes"):
> > > -${CC:-gcc} -o ext2-test ext2-test.c -lext2fs >/dev/null
2>&1
> > > +if test -z ${CC}; then CC="gcc"; fi
> > > +${CC} -o ext2-test ext2-test.c -lext2fs >/dev/null
2>&1
> > > +
> >
> > This also seems to serve no purpose as far as I can tell.
> 
> This is for the ancient Solaris /bin/sh.  If John Levon confirms that
> this and that the $(SHELL)  mentioned in your other mail 
> are NOT needed/required to not break Solaris, you can undo these two
changes.
It''s not needed for the Bourne shell, no.

regards,
john

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

On Thursday 27 March 2008 16:36:45 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage: 
portability fixes"):
> > Everytime when I submitted a patch where I changed /bin/bash to
/bin/sh
> > John Levon came up with a "Build is broken on Solaris"
message.
> > The fix was always the same: Use $(SHELL) as this is explicitely set
for
> > Solaris.
>
> People whose /bin/sh is that badly broken deserve to lose IMO - that
> is to say, we shouldn''t inconvenience other people for their
benefit.
> We''ve had this conversation before.
>
> But in any case your syntax was completely wrong and the script has
> #!/bin/sh at the top already so I don''t see why this is relevant
to
> you.
>
> > On Thursday 27 March 2008 11:54:41 Ian Jackson wrote:
> > > Christoph Egger writes ("[Xen-devel] [PATCH][TOOLS]
libfsimage:
> > > portability Please forgive my ignorance: Does NetBSD offer a
different
> > > (non-raw) device which does not have this requirement.  If so
perhaps
> > > we should be using it instead - if not, why not ?
> >
> > The raw device pass requests directly to the underlying device, with
> > only check/adjustments against the partition bounds. Especially it
won''t
> > try to do read/modify/write for write requests, or expand the read if
> > it''s not sector-aligned.
>
> Yes, but these aren''t desirable in this case.
>
> > The block device doesn''t have this restriction, but allows
only ONE open,
> > therefore it is not usable by pygrub. It also has other side-effects
> > (as it goes through the buffer cache), it''s definitively not
useable for
> > the NetBSD block device *backend*   or for qemu-dm I/O.
>
> The problem is that pygrub wants to open the disk multiple times ?
> Or is it that xend has already plumbed in the device to qemu-dm or
> what have you, and that prevents a second open ?
The block backend opens the block device before pygrub is started.
So, when you have "disk = [ ''phy:/dev/bla,blub,w'' ]"
in your setup you get
an EBUSY when pygrub tries to open the block device.
> I accept that it''s not suitable for use as the full block backend,
but
> perhaps the answer is to pass pygrub an edited version of the device
> name, or have pygrub edit it itself.  If we were to use the non-raw
> device for pygrub and the raw device for qemu-dm, would things work ?
I don''t think it would work without a lot of work on the backend
device,
if it is possible at all. For now, the backend assumes it was given a
block device.

Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy


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

Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage:
portability fixes"):
> On Thursday 27 March 2008 16:36:45 Ian Jackson wrote:
> > I accept that it''s not suitable for use as the full block
backend, but
> > perhaps the answer is to pass pygrub an edited version of the device
> > name, or have pygrub edit it itself.  If we were to use the non-raw
> > device for pygrub and the raw device for qemu-dm, would things work ?
> 
> I don''t think it would work without a lot of work on the backend
device,
> if it is possible at all. For now, the backend assumes it was given a
> block device.
I''m afraid I don''t follow this at all.  (What does `block
device''
stand in opposition to?)

Is it possible for pygrub to open the non-raw device while the block
backend is using the raw device ?  Or do you mean that the block
backend is already using the non-raw device and that you''re having
pygrub use the raw device just so that you are able to use the same
underlying storage object twice simultaneously ?

Ian.

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

On Friday 28 March 2008 11:42:15 Ian Jackson wrote:
> Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage: 
portability fixes"):
> > On Thursday 27 March 2008 16:36:45 Ian Jackson wrote:
> > > I accept that it''s not suitable for use as the full
block backend, but
> > > perhaps the answer is to pass pygrub an edited version of the
device
> > > name, or have pygrub edit it itself.  If we were to use the
non-raw
> > > device for pygrub and the raw device for qemu-dm, would things
work ?
> >
> > I don''t think it would work without a lot of work on the
backend device,
> > if it is possible at all. For now, the backend assumes it was given a
> > block device.
>
> I''m afraid I don''t follow this at all.  (What does `block
device''
> stand in opposition to?)
"block device" is what is given in the guest setup with
"disk = [ ''phy:/dev/wd0a,0x1,w'' ]"  (NetBSD syntax)
"disk = [ ''phy:/dev/hda1,hda,w'' ]" (Linux syntax)
> Is it possible for pygrub to open the non-raw device while the block
> backend is using the raw device ?  Or do you mean that the block
> backend is already using the non-raw device and that you''re having
> pygrub use the raw device just so that you are able to use the same
> underlying storage object twice simultaneously ?
With changeset 17300, pygrub uses a raw device on NetBSD only.
The block backend already opened the block device when pygrub
opens the raw device.

Christoph

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy


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

Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS] libfsimage:
portability fixes"):
> On Friday 28 March 2008 11:42:15 Ian Jackson wrote:
> > Christoph Egger writes ("Re: [Xen-devel] [PATCH][TOOLS]
libfsimage:
> portability fixes"):
> > > On Thursday 27 March 2008 16:36:45 Ian Jackson wrote:
> > > > I accept that it''s not suitable for use as the full
block backend, but
> > > > perhaps the answer is to pass pygrub an edited version of
the device
> > > > name, or have pygrub edit it itself.  If we were to use the
non-raw
> > > > device for pygrub and the raw device for qemu-dm, would
things work ?
> > >
> > > I don''t think it would work without a lot of work on the
backend device,
> > > if it is possible at all. For now, the backend assumes it was
given a
> > > block device.
> >
> > I''m afraid I don''t follow this at all.  (What does
`block device''
> > stand in opposition to?)
> 
> "block device" is what is given in the guest setup with
> "disk = [ ''phy:/dev/wd0a,0x1,w'' ]"  (NetBSD
syntax)
> "disk = [ ''phy:/dev/hda1,hda,w'' ]" (Linux
syntax)
Yes, I know that.  But you say `the backend assumes it was given a
block device'' and I was suggesting giving it the raw block device so
obviously we are talking at cross purposes.

Do you mean that it would somehow be difficult for the block backend
to talk to /dev/rwd0a instead ?  Surely that would be more appropriate
in any case.
> > Is it possible for pygrub to open the non-raw device while the block
> > backend is using the raw device ?  Or do you mean that the block
> > backend is already using the non-raw device and that you''re
having
> > pygrub use the raw device just so that you are able to use the same
> > underlying storage object twice simultaneously ?
> 
> With changeset 17300, pygrub uses a raw device on NetBSD only.
> The block backend already opened the block device when pygrub
> opens the raw device.
Yes, so you said already.  But you haven''t answered my question.

Ian.

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