Libguestfs - Nov 2018 - Re: Further nbdkit patches for Haiku

François, thanks for the patches so far.

This morning I pushed:

 
https://github.com/libguestfs/nbdkit/commit/e26fb6cea3fdba13b5472daf6767e76abacd8d03
  build: add missing #include "byte-swapping.h"

 
https://github.com/libguestfs/nbdkit/commit/b962272a561bfed087316d24b7d510e4ab29a985
  build: more fallbacks for mkostemp

The iconv patch makes iconv a requirement.  I don't know how many
systems actually lack iconv -- probably not very many -- but even so I
don't want nbdkit to require a library just to compile a single
plugin, as a feature of nbdkit is that the minimal set of
requirements.  So I have modified your patch (see attachment) to make
the iconv dependency optional.  Can you let me know if this works on
Haiku or if I broke anything?

The final patch was:

 
https://github.com/mmuman/nbdkit/commit/50ee315525258b0defae06f6e29da841ddc42b58
  build: avoid hardcoding /var/tmp by default, use P_tmpdir

Unfortunately this won't work well on some Linux systems which use the
/tmp-on-tmpdir misfeature.  On these systems, /tmp has only a small,
limited amount of space and is backed by memory, and we must use
/var/tmp for large files.  I don't know how to fix this.  Perhaps a
simple:

AS_CASE([$host_os],
  [haiku*], [LARGE_TMPDIR=/tmp]
  [LARGE_TMPDIR=/var/tmp]
)
AC_DEFINE_UNQUOTED([LARGE_TMPDIR],["$LARGE_TMPDIR"],
                   [Temporary directory for large files])

would be enough for now?

Finally about the list of failing tests:

  https://pastebin.com/1PFG85DS

Mostly these are because of the old qemu-io binary.  The qemu-io -f
option was added 4 years ago :-/ so it could be time to upgrade.

This error:

  ./test-shebang-perl.sh: PID file was not created
  rect ELF header
  FAIL test-shebang-perl.sh (exit status: 1)

(I think the error message is "incorrect ELF header") could be
something to do with the way we use shebangs (#!).  IIRC this also
fails on FreeBSD so probably nbdkit is doing something which is not
correct POSIX.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

--uIQsGlkY6qbVGkBB
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: attachment;
	filename="0001-build-add-a-simple-test-for-iconv.patch"
Content-Transfer-Encoding: 8bit
>From b5f66b866b3365f62236668638c093be426c89a3 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Tue, 6 Nov 2018 08:46:27 +0000
Subject: [PATCH] build: add a simple test for iconv
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It's used by one plugin.

There is a proper iconv.m4 available but it makes autoconf fail
if we don't bring in more of gettext, which wants to add 50 files around.

Original Author: François Revol <revol@free.fr>

RWMJ: I modified this to make the iconv dependency optional.
If unavailable, the floppy plugin is disabled.
---
 README                     |  5 +++++
 configure.ac               | 24 ++++++++++++++++++++++++
 plugins/floppy/Makefile.am |  7 ++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/README b/README
index e0c95cd..0b266f2 100644
--- a/README
+++ b/README
@@ -77,6 +77,11 @@ For the iso plugin:
 
  - genisoimage or mkisofs
 
+For the floppy plugin:
+
+ - iconv (on Linux this is built into glibc, on other systems
+   it may be a separate library)
+
 For the libvirt plugin:
 
  - libvirt
diff --git a/configure.ac b/configure.ac
index a64ae31..11a3fd2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -144,6 +144,7 @@ AC_CHECK_HEADERS([\
 	alloca.h \
 	byteswap.h \
 	endian.h \
+	iconv.h \
 	selinux/selinux.h \
 	sys/endian.h \
 	sys/prctl.h \
@@ -164,6 +165,29 @@ AC_SEARCH_LIBS([dlopen], [dl dld], [
 ])
 LIBS="$old_LIBS"
 
+old_LIBS="$LIBS"
+AC_SEARCH_LIBS([iconv], [iconv], [
+        AS_IF([test "x$ac_cv_search_iconv" != "xnone
required"],
+	    [ICONV_LIBS="$ac_cv_search_iconv"
+             have_iconv=yes
+            ],
+            [ICONV_LIBS+             have_iconv=yes])
+	], [
+        AC_SEARCH_LIBS([libiconv], [iconv], [
+                AS_IF([test "x$ac_cv_search_libiconv" != "xnone
required"],
+                [ICONV_LIBS="$ac_cv_search_libiconv"
+                 have_iconv=yes
+                ],
+                [ICONV_LIBS+                 have_iconv=yes])
+            ], [AC_MSG_WARN([unable to find a library containing iconv])
+        ])
+])
+AC_SUBST([ICONV_LIBS])
+AM_CONDITIONAL([HAVE_ICONV],[test "x$have_iconv" = "xyes"])
+LIBS="$old_LIBS"
+
 dnl Check if -rdynamic linker flag works.
 acx_nbdkit_save_LDFLAGS="${LDFLAGS}"
 LDFLAGS="${LDFLAGS} -rdynamic"
diff --git a/plugins/floppy/Makefile.am b/plugins/floppy/Makefile.am
index b6c2435..9723478 100644
--- a/plugins/floppy/Makefile.am
+++ b/plugins/floppy/Makefile.am
@@ -34,6 +34,8 @@ include $(top_srcdir)/common-rules.mk
 
 EXTRA_DIST = nbdkit-floppy-plugin.pod
 
+if HAVE_ICONV
+
 plugin_LTLIBRARIES = nbdkit-floppy-plugin.la
 
 nbdkit_floppy_plugin_la_SOURCES = \
@@ -51,7 +53,8 @@ nbdkit_floppy_plugin_la_CPPFLAGS = \
 nbdkit_floppy_plugin_la_CFLAGS = \
 	$(WARNINGS_CFLAGS)
 nbdkit_floppy_plugin_la_LDFLAGS = \
-	-module -avoid-version -shared
+	-module -avoid-version -shared \
+	$(ICONV_LIBS)
 nbdkit_floppy_plugin_la_LIBADD = \
 	$(top_builddir)/common/regions/libregions.la
 
@@ -66,3 +69,5 @@ nbdkit-floppy-plugin.1: nbdkit-floppy-plugin.pod
 	    $<
 
 endif HAVE_POD
+
+endif
-- 
2.19.0.rc0


--uIQsGlkY6qbVGkBB--

Hi,

Le 06/11/2018 à 10:02, Richard W.M. Jones a écrit :
> François, thanks for the patches so far.
> 
> This morning I pushed:
> 
>  
https://github.com/libguestfs/nbdkit/commit/e26fb6cea3fdba13b5472daf6767e76abacd8d03
>   build: add missing #include "byte-swapping.h"
> 
>  
https://github.com/libguestfs/nbdkit/commit/b962272a561bfed087316d24b7d510e4ab29a985
>   build: more fallbacks for mkostemp
> 
> The iconv patch makes iconv a requirement.  I don't know how many
> systems actually lack iconv -- probably not very many -- but even so I
> don't want nbdkit to require a library just to compile a single
> plugin, as a feature of nbdkit is that the minimal set of
> requirements.  So I have modified your patch (see attachment) to make
> the iconv dependency optional.  Can you let me know if this works on
> Haiku or if I broke anything?
Seems to work.
> The final patch was:
> 
>  
https://github.com/mmuman/nbdkit/commit/50ee315525258b0defae06f6e29da841ddc42b58
>   build: avoid hardcoding /var/tmp by default, use P_tmpdir
> 
> Unfortunately this won't work well on some Linux systems which use the
> /tmp-on-tmpdir misfeature.  On these systems, /tmp has only a small,
> limited amount of space and is backed by memory, and we must use
> /var/tmp for large files.  I don't know how to fix this.  Perhaps a
> simple:
> 
> AS_CASE([$host_os],
>   [haiku*], [LARGE_TMPDIR=/tmp]
>   [LARGE_TMPDIR=/var/tmp]
> )
> AC_DEFINE_UNQUOTED([LARGE_TMPDIR],["$LARGE_TMPDIR"],
>                    [Temporary directory for large files])
> 
> would be enough for now?
That should do.

I made a patch here:

https://github.com/mmuman/nbdkit/commits/tmpfix

Also added one for the loader lib search path environment, which is
different for us.
> Finally about the list of failing tests:
> 
>   https://pastebin.com/1PFG85DS
> 
> Mostly these are because of the old qemu-io binary.  The qemu-io -f
> option was added 4 years ago :-/ so it could be time to upgrade.
Well, our fork is quite old, and QEMU is fast-moving…
> This error:
> 
>   ./test-shebang-perl.sh: PID file was not created
>   rect ELF header
>   FAIL test-shebang-perl.sh (exit status: 1)
> 
> (I think the error message is "incorrect ELF header") could be
> something to do with the way we use shebangs (#!).  IIRC this also
> fails on FreeBSD so probably nbdkit is doing something which is not
> correct POSIX.
Yes I believe it gets rejected because the path is relative.
I think FreeBSD also probably rejects non-absolute interpreter paths for
security reasons.

François.

On Fri, Nov 09, 2018 at 01:44:38AM +0100, François Revol
wrote:
> Le 06/11/2018 à 10:02, Richard W.M. Jones a écrit :
> I made a patch here:
> 
> https://github.com/mmuman/nbdkit/commits/tmpfix
> 
> Also added one for the loader lib search path environment, which is
> different for us.
For the tmpdir patch, I changed plugins/example3/example3.c to a
simpler patch which doesn't require the variable stack allocation
(but also doesn't use TMPDIR -- it's only an example program so
I chose to keep the code simple).

I pushed the LIBRARY_PATH patch unchanged.
> > This error:
> > 
> >   ./test-shebang-perl.sh: PID file was not created
> >   rect ELF header
> >   FAIL test-shebang-perl.sh (exit status: 1)
> > 
> > (I think the error message is "incorrect ELF header") could
be
> > something to do with the way we use shebangs (#!).  IIRC this also
> > fails on FreeBSD so probably nbdkit is doing something which is not
> > correct POSIX.
> 
> Yes I believe it gets rejected because the path is relative.
> I think FreeBSD also probably rejects non-absolute interpreter paths for
> security reasons.
Interesting - I guess this wouldn't affect normal uses of shebangs,
only the tests.  This is probably fixable with a bit of effort but my
FreeBSD VM died recently so I have to get that working first.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, Nov 06, 2018 at 09:02:17AM +0000, Richard W.M. Jones
wrote:
> Finally about the list of failing tests:
> 
>   https://pastebin.com/1PFG85DS
> 
> Mostly these are because of the old qemu-io binary.  The qemu-io -f
> option was added 4 years ago :-/ so it could be time to upgrade.
FWIW this turns out to be a problem in RHEL 7 as well :-(  I worked
around it (in RHEL 7 only) by patching the affected tests like this:

  # Ancient qemu-io in RHEL 7 doesn't support -f FORMAT option.  However
  # we can just omit it and the tests still work fine.
  for f in tests/*.sh; do
    sed -i -e 's/qemu-io -f raw/qemu-io/g' $f
  done
> This error:
> 
>   ./test-shebang-perl.sh: PID file was not created
>   rect ELF header
>   FAIL test-shebang-perl.sh (exit status: 1)
> 
> (I think the error message is "incorrect ELF header") could be
> something to do with the way we use shebangs (#!).  IIRC this also
> fails on FreeBSD so probably nbdkit is doing something which is not
> correct POSIX.
I reproduced this on FreeBSD.  The path used in the tests is indeed
relative:

  #!../nbdkit

However replacing this with an absolute path did *not* fix the issue.
FreeBSD seems to not like running a shell script from a shebang,
probably because this is (was?) insecure.  However because of the way
our test harness works we really need to run the shell script.  As
there seems to be no simple way to fix this for now, I left the bug in
1.8.0.

Finally the way that iconv detection was implemented broke FreeBSD.  I
added an interim replacement for this which at least fixes FreeBSD &
Linux, however of couse I did not test Haiku:

 
https://github.com/libguestfs/nbdkit/commit/acbe7ad89e75efa8eea41d5891bca3972214200a

So hopefully nbdkit 1.8.0 works for you, or at least is not completely
broken.  If there are any problems then let us know.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Le 13/11/2018 à 14:40, Richard W.M. Jones a écrit :
> On Tue, Nov 06, 2018 at 09:02:17AM +0000, Richard W.M. Jones wrote:
>> Finally about the list of failing tests:
>>
>>   https://pastebin.com/1PFG85DS
>>
>> Mostly these are because of the old qemu-io binary.  The qemu-io -f
>> option was added 4 years ago :-/ so it could be time to upgrade.
> 
> FWIW this turns out to be a problem in RHEL 7 as well :-(  I worked
> around it (in RHEL 7 only) by patching the affected tests like this:
> 
>   # Ancient qemu-io in RHEL 7 doesn't support -f FORMAT option. 
However
>   # we can just omit it and the tests still work fine.
>   for f in tests/*.sh; do
>     sed -i -e 's/qemu-io -f raw/qemu-io/g' $f
>   done
> 

Possibly use something like this?

qemuioargs="-f raw"
qemu-io -h | grep -- -f > /dev/null 2>&1 || qemuioargs=""

qemu-io $qemuioargs ...

Seems to work here.
> I reproduced this on FreeBSD.  The path used in the tests is indeed
> relative:
> 
>   #!../nbdkit
> 
> However replacing this with an absolute path did *not* fix the issue.
> FreeBSD seems to not like running a shell script from a shebang,
> probably because this is (was?) insecure.  However because of the way
> our test harness works we really need to run the shell script.  As
> there seems to be no simple way to fix this for now, I left the bug in
> 1.8.0.
Possibly there are other restrictions, like having the interpreter
root-owned, dunno.

Looks like their ports attempt to fix those:
https://www.freebsd.org/doc/en/books/porters-handbook/uses-shebangfix.html

But the page doesn't list the restrictions.
> 
> Finally the way that iconv detection was implemented broke FreeBSD.  I
> added an interim replacement for this which at least fixes FreeBSD &
> Linux, however of couse I did not test Haiku:
> 
>  
https://github.com/libguestfs/nbdkit/commit/acbe7ad89e75efa8eea41d5891bca3972214200a
Well it doesn't work, because you don't link with the library, so the
floppy plugin will be skipped.
> So hopefully nbdkit 1.8.0 works for you, or at least is not completely
> broken.  If there are any problems then let us know.


François.

On 11/13/18 7:40 AM, Richard W.M. Jones wrote:
>> (I think the error message is "incorrect ELF header") could
be
>> something to do with the way we use shebangs (#!).  IIRC this also
>> fails on FreeBSD so probably nbdkit is doing something which is not
>> correct POSIX.
> 
POSIX says that #! shebangs are undefined behavior. You're at the mercy 
of each operating system, because there is no portable solution across 
all systems.
> I reproduced this on FreeBSD.  The path used in the tests is indeed
> relative:
> 
>    #!../nbdkit
> 
> However replacing this with an absolute path did *not* fix the issue.
> FreeBSD seems to not like running a shell script from a shebang,
> probably because this is (was?) insecure.  However because of the way
> our test harness works we really need to run the shell script.  As
> there seems to be no simple way to fix this for now, I left the bug in
> 1.8.0.
Can you use '#!/bin/sh /path/to/nbdkit'?  That way, the shebang is 
running a binary (not a script), where the binary in turn runs the 
desired script?  Or does that run into other limitations (for example, 
#! can only portably used with exactly one argument after the 
interpreter name; BSD and more recently coreutils have added 'env -S' to
work around that limitation, although it is not yet widespread 
portable).  But yes, I do seem to recall that #! must point to a binary 
interpreter rather than a script of an unspecified language (the fact 
that Linux will happily run a script under /bin/sh is not universal).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org