[PATCH 1/8] raisin: Fix non-verbose case

[PATCH 1/8] raisin: Fix non-verbose case

[George Dunlap]

Because we use "set -e", we can't use the "a && b" construct, as it will fail and stop the script.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 lib/common-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/common-functions.sh b/lib/common-functions.sh
index 36e1766..06c894a 100644
--- a/lib/common-functions.sh
+++ b/lib/common-functions.sh
@@ -232,9 +232,9 @@ function for_each_component () {
         fi
         if eval [[ ! -z \$"$capital"_REVISION ]]
         then
-            [[ $VERBOSE -eq 1 ]] && echo calling "$component"_"$1"
+            [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
             "$component"_"$1"
-            [[ $VERBOSE -eq 1 ]] && echo "$component"_"$1" done
+            [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
         fi
     done
 }
-- 
1.9.1

[PATCH 2/8] Include actual config in package, not defconfig

[George Dunlap]

We want to include the actual config used to build the packages.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 raise         | 1 -
 scripts/mkdeb | 2 +-
 scripts/mkrpm | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/raise b/raise
index 94507d2..bce6908 100755
--- a/raise
+++ b/raise
@@ -74,4 +74,3 @@ case "$1" in
 esac
 
 $COMMAND
-
diff --git a/scripts/mkdeb b/scripts/mkdeb
index 86fac72..0225736 100755
--- a/scripts/mkdeb
+++ b/scripts/mkdeb
@@ -36,7 +36,7 @@ cp -r data deb/opt/raisin
 cp -r components deb/opt/raisin
 cp -r scripts deb/opt/raisin
 cp -r lib deb/opt/raisin
-cp defconfig raise deb/opt/raisin
+cp config raise deb/opt/raisin
 
 
 # Debian doesn't use /usr/lib64 for 64-bit libraries
diff --git a/scripts/mkrpm b/scripts/mkrpm
index 98b630c..8de4508 100755
--- a/scripts/mkrpm
+++ b/scripts/mkrpm
@@ -48,7 +48,7 @@ cp -r "$BASEDIR"/data \$RPM_BUILD_ROOT/opt/raisin
 cp -r "$BASEDIR"/components \$RPM_BUILD_ROOT/opt/raisin
 cp -r "$BASEDIR"/scripts \$RPM_BUILD_ROOT/opt/raisin
 cp -r "$BASEDIR"/lib \$RPM_BUILD_ROOT/opt/raisin
-cp "$BASEDIR"/defconfig \$RPM_BUILD_ROOT/opt/raisin
+cp "$BASEDIR"/config \$RPM_BUILD_ROOT/opt/raisin
 cp "$BASEDIR"/raise \$RPM_BUILD_ROOT/opt/raisin
 
 %clean
-- 
1.9.1

[PATCH 3/8] raisin: Use fakeroot for mkdeb so we can build the package as non-root

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 lib/common-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/common-functions.sh b/lib/common-functions.sh
index 06c894a..373d6fb 100644
--- a/lib/common-functions.sh
+++ b/lib/common-functions.sh
@@ -242,7 +242,7 @@ function for_each_component () {
 function build_package() {
     if [[ $DISTRO = "Debian" ]]
     then
-        ./scripts/mkdeb "$1"
+        fakeroot bash ./scripts/mkdeb "$1"
     elif [[  $DISTRO = "Fedora" ]]
     then
         ./scripts/mkrpm "$1"
-- 
1.9.1

[PATCH 4/8] raisin: Use PKGTYPE rather than DISTRO to determine how to build a package

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 lib/common-functions.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/common-functions.sh b/lib/common-functions.sh
index 373d6fb..e66c6f4 100644
--- a/lib/common-functions.sh
+++ b/lib/common-functions.sh
@@ -239,16 +239,16 @@ function for_each_component () {
     done
 }
 
+function _build_package_deb() {
+    fakeroot bash ./scripts/mkdeb "$1"
+}
+
+function _build_package_rpm() {
+    ./scripts/mkrpm "$1"
+}
+
 function build_package() {
-    if [[ $DISTRO = "Debian" ]]
-    then
-        fakeroot bash ./scripts/mkdeb "$1"
-    elif [[  $DISTRO = "Fedora" ]]
-    then
-        ./scripts/mkrpm "$1"
-    else
-        echo "Don't know how to create packages for $DISTRO"
-    fi
+    _build_package_${PKGTYPE} "$1"
 }
 
 function install_package() {
-- 
1.9.1

[PATCH 5/8] raisin: Fix CentOS build

[George Dunlap]

Add package dependencies for CentOS.  Also use PKGTYPE rather than
DISTRO to determine if we need rpm-build.

I've tested this for xen but not for libvirt or grub.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/grub    |  5 +++++
 components/libvirt |  7 +++++++
 components/xen     | 10 ++++++++--
 lib/commands.sh    |  2 +-
 4 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/components/grub b/components/grub
index 563a28c..af396d9 100644
--- a/components/grub
+++ b/components/grub
@@ -12,6 +12,11 @@ function grub_check_package() {
     local DEP_Fedora_x86_32="$DEP_Fedora_common"
     local DEP_Fedora_x86_64="$DEP_Fedora_common glibc-devel.i686"
 
+    local DEP_CentOS_common="make gcc tar automake autoconf sysconftool bison flex \
+                             glibc-devel"
+    local DEP_CentOS_x86_32="$DEP_CentOS_common"
+    local DEP_CentOS_x86_64="$DEP_CentOS_common glibc-devel.i686"
+
 
     if [[ $ARCH != "x86_64" && $ARCH != "x86_32" ]]
     then
diff --git a/components/libvirt b/components/libvirt
index 5853950..aef1bc8 100644
--- a/components/libvirt
+++ b/components/libvirt
@@ -18,6 +18,13 @@ function libvirt_check_package() {
     local DEP_Fedora_x86_32="$DEP_Fedora_common"
     local DEP_Fedora_x86_64="$DEP_Fedora_common"
 
+    local DEP_CentOS_common="patch make gcc libtool autoconf gettext-devel     \
+                             python-devel libxslt yajl-devel libxml2-devel     \
+                             device-mapper-devel libpciaccess-devel            \
+                             libuuid-devel perl-XML-XPath"
+    local DEP_CentOS_x86_32="$DEP_CentOS_common"
+    local DEP_CentOS_x86_64="$DEP_CentOS_common"
+
     echo Checking Libvirt dependencies
     eval check-package \$DEP_"$DISTRO"_"$ARCH"
 }
diff --git a/components/xen b/components/xen
index a0c0034..f2e1254 100644
--- a/components/xen
+++ b/components/xen
@@ -11,10 +11,16 @@ function xen_check_package() {
 
     local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
              ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
-             patch pixman-devel glibc-devel bridge-utils grub2 wget"
-    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
+             patch pixman-devel glibc-devel bridge-utils grub2 wget tar bzip2"
+    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 texinfo acpica-devel"
     local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
 
+    local DEP_CentOS_common="make gcc python-devel gettext libuuid-devel   \
+             ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
+             patch pixman-devel glibc-devel bridge-utils wget tar bzip2"
+    local DEP_CentOS_x86_32="$DEP_CentOS_common dev86 texinfo iasl"
+    local DEP_CentOS_x86_64="$DEP_CentOS_x86_32 glibc-devel.i686"
+
     echo Checking Xen dependencies
     eval check-package \$DEP_"$DISTRO"_"$ARCH"
 }
diff --git a/lib/commands.sh b/lib/commands.sh
index c47ba1f..17e2129 100755
--- a/lib/commands.sh
+++ b/lib/commands.sh
@@ -5,7 +5,7 @@ function check-builddep() {
 
     check-package git
 
-    if [[ $DISTRO = "Fedora" ]]
+    if [[ $PKGTYPE = "rpm" ]]
     then
         check-package rpm-build
     fi
-- 
1.9.1

[PATCH 6/8] raisin: Break build into components and allow the sub-steps to be called individually

[George Dunlap]

This makes it easier to debug just one aspect of the build.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 lib/commands.sh | 16 ++++++++++++----
 raise           |  2 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/lib/commands.sh b/lib/commands.sh
index 17e2129..c5df9ad 100755
--- a/lib/commands.sh
+++ b/lib/commands.sh
@@ -45,17 +45,25 @@ function install-builddep() {
     YES=y check-builddep
 }
 
-function build() {
-    check-builddep
-    
+function build-only() {
     mkdir -p "$INST_DIR" &>/dev/null
 
     # build and install under $DESTDIR
     for_each_component build
-    
+}
+
+function build-package() {
     build_package xen-system
 }
 
+function build() {
+    check-builddep
+
+    build-only
+
+    build-package
+}
+
 function unraise() {
     for_each_component clean
 
diff --git a/raise b/raise
index bce6908..59de04a 100755
--- a/raise
+++ b/raise
@@ -64,7 +64,7 @@ do
 done
 
 case "$1" in
-    "install-builddep" | "build" | "install" | "configure" | "unraise" )
+    "install-builddep" | "build" | "build-only" | "build-package" | "install" | "configure" | "unraise" )
         COMMAND=$1
         ;;
     *)
-- 
1.9.1

[PATCH 7/8] raisin: Rework component specification

[George Dunlap]

Allow COMPONENTS to be specified in the config (or on the command-line)

Now you can keep all components enabled in your config but build only
one like so:

COMPONENTS="xen" ./raise build

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 defconfig               |  5 +++++
 lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/defconfig b/defconfig
index 23c76eb..4ec8a0b 100644
--- a/defconfig
+++ b/defconfig
@@ -1,5 +1,10 @@
 # Config variables for raisin
 
+# Components
+# All components: xen grub libvirt
+# Core xen functionality: xen
+DEFAULT_COMPONENTS="xen grub libvirt"
+
 # Build config
 ## Make command to run
 MAKE="make -j2"
diff --git a/lib/common-functions.sh b/lib/common-functions.sh
index e66c6f4..42406e9 100644
--- a/lib/common-functions.sh
+++ b/lib/common-functions.sh
@@ -31,13 +31,41 @@ function common_init() {
 
     get_distro
     get_arch
+    get_components
 
-    for f in `cat "$BASEDIR"/components/series`
+    echo "Distro: $DISTRO"
+    echo "Arch: $ARCH"
+    echo "Components: $COMPONENTS"
+
+    for f in $COMPONENTS
     do
         source "$BASEDIR"/components/"$f"
     done
 }
 
+function get_components() {
+    if [[ -z "$COMPONENTS" ]]
+    then
+	COMPONENTS="$DEFAULT_COMPONENTS"
+    fi
+
+    if [[ -z "$COMPONENTS" ]] 
+    then 
+	local component
+	for component in `cat "$BASEDIR"/components/series`
+	do
+	    local capital
+            capital=`echo $component | tr '[:lower:]' '[:upper:]'`
+            if eval [[ ! -z \$"$capital"_UPSTREAM_REVISION ]]
+            then
+		COMPONENTS="$COMPONENTS $component"
+		echo "Found component $component"
+            fi
+	done
+	export COMPONENTS
+    fi
+}
+
 function get_distro() {
     if [[ -x "`which lsb_release 2>/dev/null`" ]]
     then
@@ -222,20 +250,13 @@ function stop_initscripts() {
 }
 
 function for_each_component () {
-    for component in `cat "$BASEDIR"/components/series`
+    local component
+
+    for component in $COMPONENTS
     do
-        capital=`echo $component | tr '[:lower:]' '[:upper:]'`
-        if [[ $VERBOSE -eq 1 ]]
-        then
-            echo -n "$capital"_REVISION =" "
-            eval echo \$"$capital"_REVISION
-        fi
-        if eval [[ ! -z \$"$capital"_REVISION ]]
-        then
-            [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
-            "$component"_"$1"
-            [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
-        fi
+        [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
+        "$component"_"$1"
+        [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
     done
 }
 
-- 
1.9.1

[PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

Until we start hosting the blktap repo on xenbits, use the one from github.

Also, we need to pass in the directories where to find the
libblktapctl headers in the Xen build.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Note: For now use the "upstream" XenServer repo.

Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
which can be found here:

git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2

CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 components/series |  1 +
 components/xen    |  5 +++++
 defconfig         |  9 ++++++---
 4 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100644 components/blktap

diff --git a/components/blktap b/components/blktap
new file mode 100644
index 0000000..3062b2a
--- /dev/null
+++ b/components/blktap
@@ -0,0 +1,47 @@
+#!/usr/bin/env bash
+
+function blktap_check_package() {
+    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
+    local DEP_Debian_x86_32="$DEP_Debian_common"
+    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
+    local DEP_Debian_arm32="$DEP_Debian_common"
+    local DEP_Debian_arm64="$DEP_Debian_arm32"
+
+    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
+    local DEP_Fedora_x86_32="$DEP_Fedora_common"
+    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
+
+    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
+    local DEP_CentOS_x86_32="$DEP_Redhat_common"
+    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
+
+    echo Checking blktap dependencies
+    eval check-package \$DEP_"$DISTRO"_"$ARCH"
+}
+
+function blktap_build() {
+    cd "$BASEDIR"
+    git-checkout $BLKTAP_URL $BLKTAP_REVISION blktap-dir
+    cd blktap-dir
+    ./autogen.sh
+    ./configure --prefix=$PREFIX
+    $MAKE
+    $MAKE install DESTDIR="$INST_DIR"
+    cd "$BASEDIR"
+}
+
+function blktap_clean() {
+    cd "$BASEDIR"
+    if [[ -d blktap-dir ]]
+    then
+	rm -rf blktap-dir
+    fi
+}
+
+function blktap_configure() {
+    :
+}
+
+function blktap_unconfigure() {
+    :
+}
diff --git a/components/series b/components/series
index 8f614f0..953800a 100644
--- a/components/series
+++ b/components/series
@@ -1,3 +1,4 @@
+blktap
 xen
 qemu
 grub
diff --git a/components/xen b/components/xen
index f2e1254..03a1970 100644
--- a/components/xen
+++ b/components/xen
@@ -29,7 +29,12 @@ function xen_build() {
     cd "$BASEDIR"
     git-checkout $XEN_URL $XEN_REVISION xen-dir
     cd xen-dir
+    CFLAGS="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
+    LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
     ./configure --prefix=$PREFIX --with-system-qemu=/usr/bin/qemu-system-i386
+    CFLAGS_libblktapctl="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
+    LDLIBS_libblktapctl="-L$INST_DIR/$PREFIX/lib -lblktapctl" \
+    SHLIB_libblktapctl="-Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
     $RAISIN_MAKE
     $RAISIN_MAKE install DESTDIR="$INST_DIR"
     cd "$BASEDIR"
diff --git a/defconfig b/defconfig
index 4ec8a0b..283e954 100644
--- a/defconfig
+++ b/defconfig
@@ -1,9 +1,9 @@
 # Config variables for raisin
 
 # Components
-# All components: xen grub libvirt
-# Core xen functionality: xen
-DEFAULT_COMPONENTS="xen grub libvirt"
+# All components: blktap xen grub libvirt
+# Core xen functionality: xen blktap
+DEFAULT_COMPONENTS="blktap xen grub libvirt"
 
 # Build config
 ## Make command to run
@@ -22,6 +22,8 @@ XEN_URL="git://xenbits.xen.org/xen.git"
 QEMU_URL="git://git.qemu.org/qemu.git"
 GRUB_URL="git://git.savannah.gnu.org/grub.git"
 LIBVIRT_URL="git://libvirt.org/libvirt.git"
+BLKTAP_URL=https://github.com/xapi-project/blktap.git
+#BLKTAP_URL=git://xenbits.xen.org/blktap.git
 
 # Software versions. Leave blank if you want to avoid the build, like
 # this: GRUB_REVISION=
@@ -30,3 +32,4 @@ XEN_REVISION="master"
 QEMU_REVISION="master"
 GRUB_REVISION="master"
 LIBVIRT_REVISION="master"
+BLKTAP_REVISION="blktap2"
-- 
1.9.1

Re: [PATCH 5/8] raisin: Fix CentOS build

[George Dunlap]

On 04/16/2015 05:19 PM, George Dunlap wrote:
> diff --git a/components/xen b/components/xen
> index a0c0034..f2e1254 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -11,10 +11,16 @@ function xen_check_package() {
>  
>      local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
>               ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
> -             patch pixman-devel glibc-devel bridge-utils grub2 wget"
> -    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
> +             patch pixman-devel glibc-devel bridge-utils grub2 wget tar bzip2"
> +    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 texinfo acpica-devel"

Sorry -- that last one should be "acpica-tools", not acpica-devel.  I
thought I'd tested it... :-/

 -George

Re: [PATCH 3/8] raisin: Use fakeroot for mkdeb so we can build the package as non-root

[Ian Campbell]

On Thu, 2015-04-16 at 17:19 +0100, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

AIUI raisin does build dependency handling/tracking. If so then fakeroot
ought to be added to it I think?

> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>

Does CC below a CC work? Interesting.

> ---
>  lib/common-functions.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> index 06c894a..373d6fb 100644
> --- a/lib/common-functions.sh
> +++ b/lib/common-functions.sh
> @@ -242,7 +242,7 @@ function for_each_component () {
>  function build_package() {
>      if [[ $DISTRO = "Debian" ]]
>      then
> -        ./scripts/mkdeb "$1"
> +        fakeroot bash ./scripts/mkdeb "$1"
>      elif [[  $DISTRO = "Fedora" ]]
>      then
>          ./scripts/mkrpm "$1"

Re: [PATCH 3/8] raisin: Use fakeroot for mkdeb so we can build the package as non-root

[Stefano Stabellini]

On Fri, 17 Apr 2015, Ian Campbell wrote:
> On Thu, 2015-04-16 at 17:19 +0100, George Dunlap wrote:
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> AIUI raisin does build dependency handling/tracking. If so then fakeroot
> ought to be added to it I think?

Good point! Please add fakeroot as Debian specific dependency for every
component, like rpm-build for Fedora.


> > ---
> > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> Does CC below a CC work? Interesting.
> 
> > ---
> >  lib/common-functions.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> > index 06c894a..373d6fb 100644
> > --- a/lib/common-functions.sh
> > +++ b/lib/common-functions.sh
> > @@ -242,7 +242,7 @@ function for_each_component () {
> >  function build_package() {
> >      if [[ $DISTRO = "Debian" ]]
> >      then
> > -        ./scripts/mkdeb "$1"
> > +        fakeroot bash ./scripts/mkdeb "$1"
> >      elif [[  $DISTRO = "Fedora" ]]
> >      then
> >          ./scripts/mkrpm "$1"
> 
> 

Re: [PATCH 1/8] raisin: Fix non-verbose case

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Because we use "set -e", we can't use the "a && b" construct, as it will fail and stop the script.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

I am wondering whether we should ban both a && b and a || b from the
code and just go with the more verbose but also harder to get wrong:

if [[ $VERBOSE -eq 1 ]]
then
    echo calling "$component"_"$1"
fi

Reading the code and seeing a || b will tempt you into writing a && b.


>  lib/common-functions.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> index 36e1766..06c894a 100644
> --- a/lib/common-functions.sh
> +++ b/lib/common-functions.sh
> @@ -232,9 +232,9 @@ function for_each_component () {
>          fi
>          if eval [[ ! -z \$"$capital"_REVISION ]]
>          then
> -            [[ $VERBOSE -eq 1 ]] && echo calling "$component"_"$1"
> +            [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
>              "$component"_"$1"
> -            [[ $VERBOSE -eq 1 ]] && echo "$component"_"$1" done
> +            [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
>          fi
>      done
>  }
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 4/8] raisin: Use PKGTYPE rather than DISTRO to determine how to build a package

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  lib/common-functions.sh | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> index 373d6fb..e66c6f4 100644
> --- a/lib/common-functions.sh
> +++ b/lib/common-functions.sh
> @@ -239,16 +239,16 @@ function for_each_component () {
>      done
>  }
>  
> +function _build_package_deb() {
> +    fakeroot bash ./scripts/mkdeb "$1"
> +}
> +
> +function _build_package_rpm() {
> +    ./scripts/mkrpm "$1"
> +}
> +
>  function build_package() {
> -    if [[ $DISTRO = "Debian" ]]
> -    then
> -        fakeroot bash ./scripts/mkdeb "$1"
> -    elif [[  $DISTRO = "Fedora" ]]
> -    then
> -        ./scripts/mkrpm "$1"
> -    else
> -        echo "Don't know how to create packages for $DISTRO"
> -    fi
> +    _build_package_${PKGTYPE} "$1"

Just use "$PKGTYPE" instead of ${}
Also please document PKGTYPE among the exported global variables in the
README.


>  }
>  
>  function install_package() {
> -- 
> 1.9.1
> 

Re: [PATCH 2/8] Include actual config in package, not defconfig

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> We want to include the actual config used to build the packages.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked and committed


> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  raise         | 1 -
>  scripts/mkdeb | 2 +-
>  scripts/mkrpm | 2 +-
>  3 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/raise b/raise
> index 94507d2..bce6908 100755
> --- a/raise
> +++ b/raise
> @@ -74,4 +74,3 @@ case "$1" in
>  esac
>  
>  $COMMAND
> -
> diff --git a/scripts/mkdeb b/scripts/mkdeb
> index 86fac72..0225736 100755
> --- a/scripts/mkdeb
> +++ b/scripts/mkdeb
> @@ -36,7 +36,7 @@ cp -r data deb/opt/raisin
>  cp -r components deb/opt/raisin
>  cp -r scripts deb/opt/raisin
>  cp -r lib deb/opt/raisin
> -cp defconfig raise deb/opt/raisin
> +cp config raise deb/opt/raisin
>  
>  
>  # Debian doesn't use /usr/lib64 for 64-bit libraries
> diff --git a/scripts/mkrpm b/scripts/mkrpm
> index 98b630c..8de4508 100755
> --- a/scripts/mkrpm
> +++ b/scripts/mkrpm
> @@ -48,7 +48,7 @@ cp -r "$BASEDIR"/data \$RPM_BUILD_ROOT/opt/raisin
>  cp -r "$BASEDIR"/components \$RPM_BUILD_ROOT/opt/raisin
>  cp -r "$BASEDIR"/scripts \$RPM_BUILD_ROOT/opt/raisin
>  cp -r "$BASEDIR"/lib \$RPM_BUILD_ROOT/opt/raisin
> -cp "$BASEDIR"/defconfig \$RPM_BUILD_ROOT/opt/raisin
> +cp "$BASEDIR"/config \$RPM_BUILD_ROOT/opt/raisin
>  cp "$BASEDIR"/raise \$RPM_BUILD_ROOT/opt/raisin
>  
>  %clean
> -- 
> 1.9.1
> 

Re: [PATCH 5/8] raisin: Fix CentOS build

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Add package dependencies for CentOS.  Also use PKGTYPE rather than
> DISTRO to determine if we need rpm-build.
> 
> I've tested this for xen but not for libvirt or grub.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/grub    |  5 +++++
>  components/libvirt |  7 +++++++
>  components/xen     | 10 ++++++++--
>  lib/commands.sh    |  2 +-
>  4 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/components/grub b/components/grub
> index 563a28c..af396d9 100644
> --- a/components/grub
> +++ b/components/grub
> @@ -12,6 +12,11 @@ function grub_check_package() {
>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
>      local DEP_Fedora_x86_64="$DEP_Fedora_common glibc-devel.i686"
>  
> +    local DEP_CentOS_common="make gcc tar automake autoconf sysconftool bison flex \
> +                             glibc-devel"
> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
> +    local DEP_CentOS_x86_64="$DEP_CentOS_common glibc-devel.i686"

Given that they are the same as Fedora, I think it is OK to:

local DEP_CentOS_common="$DEP_Fedora_common"



>  
>      if [[ $ARCH != "x86_64" && $ARCH != "x86_32" ]]
>      then
> diff --git a/components/libvirt b/components/libvirt
> index 5853950..aef1bc8 100644
> --- a/components/libvirt
> +++ b/components/libvirt
> @@ -18,6 +18,13 @@ function libvirt_check_package() {
>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
>      local DEP_Fedora_x86_64="$DEP_Fedora_common"
>  
> +    local DEP_CentOS_common="patch make gcc libtool autoconf gettext-devel     \
> +                             python-devel libxslt yajl-devel libxml2-devel     \
> +                             device-mapper-devel libpciaccess-devel            \
> +                             libuuid-devel perl-XML-XPath"
> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
> +    local DEP_CentOS_x86_64="$DEP_CentOS_common"

Same here, also please test the libvirt build: the list of dependencies
is pretty big, I worry that one of them might actually differ from Fedora


>      echo Checking Libvirt dependencies
>      eval check-package \$DEP_"$DISTRO"_"$ARCH"
>  }
> diff --git a/components/xen b/components/xen
> index a0c0034..f2e1254 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -11,10 +11,16 @@ function xen_check_package() {
>  
>      local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
>               ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
> -             patch pixman-devel glibc-devel bridge-utils grub2 wget"
> -    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
> +             patch pixman-devel glibc-devel bridge-utils grub2 wget tar bzip2"
> +    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 texinfo acpica-devel"
>      local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
>  
> +    local DEP_CentOS_common="make gcc python-devel gettext libuuid-devel   \
> +             ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
> +             patch pixman-devel glibc-devel bridge-utils wget tar bzip2"
> +    local DEP_CentOS_x86_32="$DEP_CentOS_common dev86 texinfo iasl"
> +    local DEP_CentOS_x86_64="$DEP_CentOS_x86_32 glibc-devel.i686"
> +
>      echo Checking Xen dependencies
>      eval check-package \$DEP_"$DISTRO"_"$ARCH"
>  }

I think is fair to DEP_CentOS_common="$DEP_Fedora_common" here too (once
the acpica-devel/tools thing is fixed)


> diff --git a/lib/commands.sh b/lib/commands.sh
> index c47ba1f..17e2129 100755
> --- a/lib/commands.sh
> +++ b/lib/commands.sh
> @@ -5,7 +5,7 @@ function check-builddep() {
>  
>      check-package git
>  
> -    if [[ $DISTRO = "Fedora" ]]
> +    if [[ $PKGTYPE = "rpm" ]]
>      then
>          check-package rpm-build
>      fi

Re: [PATCH 6/8] raisin: Break build into components and allow the sub-steps to be called individually

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> This makes it easier to debug just one aspect of the build.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  lib/commands.sh | 16 ++++++++++++----
>  raise           |  2 +-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/commands.sh b/lib/commands.sh
> index 17e2129..c5df9ad 100755
> --- a/lib/commands.sh
> +++ b/lib/commands.sh
> @@ -45,17 +45,25 @@ function install-builddep() {
>      YES=y check-builddep
>  }
>  
> -function build() {
> -    check-builddep
> -    
> +function build-only() {
>      mkdir -p "$INST_DIR" &>/dev/null
>  
>      # build and install under $DESTDIR
>      for_each_component build
> -    
> +}
> +
> +function build-package() {
>      build_package xen-system
>  }
>  
> +function build() {
> +    check-builddep
> +
> +    build-only
> +
> +    build-package
> +}
> +
>  function unraise() {
>      for_each_component clean
>  
> diff --git a/raise b/raise
> index bce6908..59de04a 100755
> --- a/raise
> +++ b/raise
> @@ -64,7 +64,7 @@ do
>  done
>  
>  case "$1" in
> -    "install-builddep" | "build" | "install" | "configure" | "unraise" )
> +    "install-builddep" | "build" | "build-only" | "build-package" | "install" | "configure" | "unraise" )
>          COMMAND=$1
>          ;;
>      *)

You have added two new commands, but they are not documented in the help
function or in the README. As they stand they are "hidden" commands. Is
that on purpose because they are supposed to be used just for debugging?
Maybe it would be worth documenting debugging functions too somewhere.

Re: [PATCH 7/8] raisin: Rework component specification

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Allow COMPONENTS to be specified in the config (or on the command-line)
> 
> Now you can keep all components enabled in your config but build only
> one like so:
> 
> COMPONENTS="xen" ./raise build
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Good idea, I wanted to fix the way we disable components for a while now


> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  defconfig               |  5 +++++
>  lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/defconfig b/defconfig
> index 23c76eb..4ec8a0b 100644
> --- a/defconfig
> +++ b/defconfig
> @@ -1,5 +1,10 @@
>  # Config variables for raisin
>  
> +# Components
> +# All components: xen grub libvirt
> +# Core xen functionality: xen
> +DEFAULT_COMPONENTS="xen grub libvirt"

I would just call this COMPONENTS, also see below.
Also please update the other comment in this file on how to disable components.


>  # Build config
>  ## Make command to run
>  MAKE="make -j2"
> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> index e66c6f4..42406e9 100644
> --- a/lib/common-functions.sh
> +++ b/lib/common-functions.sh
> @@ -31,13 +31,41 @@ function common_init() {
>  
>      get_distro
>      get_arch
> +    get_components
>  
> -    for f in `cat "$BASEDIR"/components/series`
> +    echo "Distro: $DISTRO"
> +    echo "Arch: $ARCH"
> +    echo "Components: $COMPONENTS"

if [[ $VERBOSE -eq 1 ]]
then
    echo stuff
fi

To make the code nicer to read, we could have a function called
verbose_echo:

function verbose_echo() {
    if [[ $VERBOSE -eq 1 ]]
    then
        echo $*
    fi
}


> +    for f in $COMPONENTS
>      do
>          source "$BASEDIR"/components/"$f"
>      done
>  }
>  
> +function get_components() {
> +    if [[ -z "$COMPONENTS" ]]
> +    then
> +	COMPONENTS="$DEFAULT_COMPONENTS"

Stray tab.
I would call our internal components variable RAISIN_COMPONENTS and the
one in the config and exposed to users COMPONENTS.


> +    fi
> +
> +    if [[ -z "$COMPONENTS" ]] 
> +    then 
> +	local component
> +	for component in `cat "$BASEDIR"/components/series`
> +	do
> +	    local capital
> +            capital=`echo $component | tr '[:lower:]' '[:upper:]'`
> +            if eval [[ ! -z \$"$capital"_UPSTREAM_REVISION ]]

please rebase: _UPSTREAM is gone


> +            then
> +		COMPONENTS="$COMPONENTS $component"
> +		echo "Found component $component"

verbose_echo


> +            fi
> +	done
> +	export COMPONENTS

a bad mix of spaces and tabs in this function


> +    fi
> +}
> +
>  function get_distro() {
>      if [[ -x "`which lsb_release 2>/dev/null`" ]]
>      then
> @@ -222,20 +250,13 @@ function stop_initscripts() {
>  }
>  
>  function for_each_component () {
> -    for component in `cat "$BASEDIR"/components/series`
> +    local component
> +
> +    for component in $COMPONENTS
>      do
> -        capital=`echo $component | tr '[:lower:]' '[:upper:]'`
> -        if [[ $VERBOSE -eq 1 ]]
> -        then
> -            echo -n "$capital"_REVISION =" "
> -            eval echo \$"$capital"_REVISION
> -        fi
> -        if eval [[ ! -z \$"$capital"_REVISION ]]
> -        then
> -            [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
> -            "$component"_"$1"
> -            [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
> -        fi
> +        [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
> +        "$component"_"$1"
> +        [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
>      done
>  }
>  
> -- 
> 1.9.1
> 

Re: [PATCH 7/8] raisin: Rework component specification

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Allow COMPONENTS to be specified in the config (or on the command-line)
> 
> Now you can keep all components enabled in your config but build only
> one like so:
> 
> COMPONENTS="xen" ./raise build
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  defconfig               |  5 +++++
>  lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/defconfig b/defconfig
> index 23c76eb..4ec8a0b 100644
> --- a/defconfig
> +++ b/defconfig
> @@ -1,5 +1,10 @@
>  # Config variables for raisin
>  
> +# Components
> +# All components: xen grub libvirt
> +# Core xen functionality: xen
> +DEFAULT_COMPONENTS="xen grub libvirt"
> +
>  # Build config
>  ## Make command to run
>  MAKE="make -j2"
> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> index e66c6f4..42406e9 100644
> --- a/lib/common-functions.sh
> +++ b/lib/common-functions.sh
> @@ -31,13 +31,41 @@ function common_init() {
>  
>      get_distro
>      get_arch
> +    get_components
>  
> -    for f in `cat "$BASEDIR"/components/series`
> +    echo "Distro: $DISTRO"
> +    echo "Arch: $ARCH"
> +    echo "Components: $COMPONENTS"
> +
> +    for f in $COMPONENTS
>      do
>          source "$BASEDIR"/components/"$f"
>      done
>  }
>  
> +function get_components() {
> +    if [[ -z "$COMPONENTS" ]]
> +    then
> +	COMPONENTS="$DEFAULT_COMPONENTS"
> +    fi
> +
> +    if [[ -z "$COMPONENTS" ]] 
> +    then 
> +	local component
> +	for component in `cat "$BASEDIR"/components/series`
> +	do
> +	    local capital
> +            capital=`echo $component | tr '[:lower:]' '[:upper:]'`
> +            if eval [[ ! -z \$"$capital"_UPSTREAM_REVISION ]]
> +            then
> +		COMPONENTS="$COMPONENTS $component"
> +		echo "Found component $component"
> +            fi
> +	done
> +	export COMPONENTS
> +    fi
> +}
> +
>  function get_distro() {
>      if [[ -x "`which lsb_release 2>/dev/null`" ]]
>      then
> @@ -222,20 +250,13 @@ function stop_initscripts() {
>  }
>  
>  function for_each_component () {
> -    for component in `cat "$BASEDIR"/components/series`
> +    local component

One important thing here is that the order is important: xen is the
first item in the series file for a reason. I think we should use the
COMPONENTS variable to check for what components are enabled, but then
still use the series file for the ordering.


> +    for component in $COMPONENTS
>      do
> -        capital=`echo $component | tr '[:lower:]' '[:upper:]'`
> -        if [[ $VERBOSE -eq 1 ]]
> -        then
> -            echo -n "$capital"_REVISION =" "
> -            eval echo \$"$capital"_REVISION
> -        fi
> -        if eval [[ ! -z \$"$capital"_REVISION ]]
> -        then
> -            [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
> -            "$component"_"$1"
> -            [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
> -        fi
> +        [[ $VERBOSE -eq 0 ]] || echo calling "$component"_"$1"
> +        "$component"_"$1"
> +        [[ $VERBOSE -eq 0 ]] || echo "$component"_"$1" done
>      done
>  }
>  
> -- 
> 1.9.1
> 

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Until we start hosting the blktap repo on xenbits, use the one from github.
> 
> Also, we need to pass in the directories where to find the
> libblktapctl headers in the Xen build.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Note: For now use the "upstream" XenServer repo.
> 
> Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
> which can be found here:
> 
> git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2
> 
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  components/series |  1 +
>  components/xen    |  5 +++++
>  defconfig         |  9 ++++++---
>  4 files changed, 59 insertions(+), 3 deletions(-)
>  create mode 100644 components/blktap
> 
> diff --git a/components/blktap b/components/blktap
> new file mode 100644
> index 0000000..3062b2a
> --- /dev/null
> +++ b/components/blktap
> @@ -0,0 +1,47 @@
> +#!/usr/bin/env bash
> +
> +function blktap_check_package() {
> +    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
> +    local DEP_Debian_x86_32="$DEP_Debian_common"
> +    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
> +    local DEP_Debian_arm32="$DEP_Debian_common"
> +    local DEP_Debian_arm64="$DEP_Debian_arm32"
> +
> +    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> +    local DEP_Fedora_x86_32="$DEP_Fedora_common"
> +    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
> +
> +    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"

DEP_CentOS_common="$DEP_Fedora_common"


> +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
> +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"

Redhat? I don't know no Redhat.


> +    echo Checking blktap dependencies
> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
> +}
> +
> +function blktap_build() {
> +    cd "$BASEDIR"
> +    git-checkout $BLKTAP_URL $BLKTAP_REVISION blktap-dir
> +    cd blktap-dir
> +    ./autogen.sh
> +    ./configure --prefix=$PREFIX
> +    $MAKE
> +    $MAKE install DESTDIR="$INST_DIR"
> +    cd "$BASEDIR"
> +}
> +
> +function blktap_clean() {
> +    cd "$BASEDIR"
> +    if [[ -d blktap-dir ]]
> +    then
> +	rm -rf blktap-dir

Stray tabs.  Do we need the emacs config stanza somewhere in raisin?


> +    fi
> +}
> +
> +function blktap_configure() {
> +    :
> +}
> +
> +function blktap_unconfigure() {
> +    :
> +}

Much better than my echo. Maybe submit a patch to replace useless echo
in configure and unconfigure functions with :


> diff --git a/components/series b/components/series
> index 8f614f0..953800a 100644
> --- a/components/series
> +++ b/components/series
> @@ -1,3 +1,4 @@
> +blktap
>  xen

I take this is on purpose so that blktap is built before xen


>  qemu
>  grub
> diff --git a/components/xen b/components/xen
> index f2e1254..03a1970 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -29,7 +29,12 @@ function xen_build() {
>      cd "$BASEDIR"
>      git-checkout $XEN_URL $XEN_REVISION xen-dir
>      cd xen-dir
> +    CFLAGS="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
> +    LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
>      ./configure --prefix=$PREFIX --with-system-qemu=/usr/bin/qemu-system-i386
> +    CFLAGS_libblktapctl="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
> +    LDLIBS_libblktapctl="-L$INST_DIR/$PREFIX/lib -lblktapctl" \
> +    SHLIB_libblktapctl="-Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \

Does this work even if blktap is actually disabled in the raisin build?
If so, then fine.

Actually I think that the xen configure script should be able to figure
out whether it can pick up blktap from an external location
automatically with the CFLAGS and LDFLAGS you set.


>      $RAISIN_MAKE
>      $RAISIN_MAKE install DESTDIR="$INST_DIR"
>      cd "$BASEDIR"
> diff --git a/defconfig b/defconfig
> index 4ec8a0b..283e954 100644
> --- a/defconfig
> +++ b/defconfig
> @@ -1,9 +1,9 @@
>  # Config variables for raisin
>  
>  # Components
> -# All components: xen grub libvirt
> -# Core xen functionality: xen
> -DEFAULT_COMPONENTS="xen grub libvirt"
> +# All components: blktap xen grub libvirt
> +# Core xen functionality: xen blktap
> +DEFAULT_COMPONENTS="blktap xen grub libvirt"
>  
>  # Build config
>  ## Make command to run
> @@ -22,6 +22,8 @@ XEN_URL="git://xenbits.xen.org/xen.git"
>  QEMU_URL="git://git.qemu.org/qemu.git"
>  GRUB_URL="git://git.savannah.gnu.org/grub.git"
>  LIBVIRT_URL="git://libvirt.org/libvirt.git"
> +BLKTAP_URL=https://github.com/xapi-project/blktap.git
> +#BLKTAP_URL=git://xenbits.xen.org/blktap.git
>  
>  # Software versions. Leave blank if you want to avoid the build, like
>  # this: GRUB_REVISION=
> @@ -30,3 +32,4 @@ XEN_REVISION="master"
>  QEMU_REVISION="master"
>  GRUB_REVISION="master"
>  LIBVIRT_REVISION="master"
> +BLKTAP_REVISION="blktap2"
> -- 
> 1.9.1
> 

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Thu, 16 Apr 2015, George Dunlap wrote:
> Until we start hosting the blktap repo on xenbits, use the one from github.
> 
> Also, we need to pass in the directories where to find the
> libblktapctl headers in the Xen build.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Note: For now use the "upstream" XenServer repo.
> 
> Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
> which can be found here:
> 
> git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2
> 
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  components/series |  1 +
>  components/xen    |  5 +++++
>  defconfig         |  9 ++++++---
>  4 files changed, 59 insertions(+), 3 deletions(-)
>  create mode 100644 components/blktap
> 
> diff --git a/components/blktap b/components/blktap
> new file mode 100644
> index 0000000..3062b2a
> --- /dev/null
> +++ b/components/blktap
> @@ -0,0 +1,47 @@
> +#!/usr/bin/env bash
> +
> +function blktap_check_package() {
> +    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
> +    local DEP_Debian_x86_32="$DEP_Debian_common"
> +    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
> +    local DEP_Debian_arm32="$DEP_Debian_common"
> +    local DEP_Debian_arm64="$DEP_Debian_arm32"
> +
> +    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> +    local DEP_Fedora_x86_32="$DEP_Fedora_common"
> +    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
> +
> +    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
> +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
> +
> +    echo Checking blktap dependencies
> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
> +}
> +
> +function blktap_build() {
> +    cd "$BASEDIR"
> +    git-checkout $BLKTAP_URL $BLKTAP_REVISION blktap-dir
> +    cd blktap-dir
> +    ./autogen.sh
> +    ./configure --prefix=$PREFIX
> +    $MAKE
> +    $MAKE install DESTDIR="$INST_DIR"
> +    cd "$BASEDIR"
> +}

I think we need to disable the build on architectures other than x86,
see grub for example


> +function blktap_clean() {
> +    cd "$BASEDIR"
> +    if [[ -d blktap-dir ]]
> +    then
> +	rm -rf blktap-dir
> +    fi
> +}
> +
> +function blktap_configure() {
> +    :
> +}
> +
> +function blktap_unconfigure() {
> +    :
> +}
> diff --git a/components/series b/components/series
> index 8f614f0..953800a 100644
> --- a/components/series
> +++ b/components/series
> @@ -1,3 +1,4 @@
> +blktap
>  xen
>  qemu
>  grub
> diff --git a/components/xen b/components/xen
> index f2e1254..03a1970 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -29,7 +29,12 @@ function xen_build() {
>      cd "$BASEDIR"
>      git-checkout $XEN_URL $XEN_REVISION xen-dir
>      cd xen-dir
> +    CFLAGS="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
> +    LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
>      ./configure --prefix=$PREFIX --with-system-qemu=/usr/bin/qemu-system-i386
> +    CFLAGS_libblktapctl="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
> +    LDLIBS_libblktapctl="-L$INST_DIR/$PREFIX/lib -lblktapctl" \
> +    SHLIB_libblktapctl="-Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
>      $RAISIN_MAKE
>      $RAISIN_MAKE install DESTDIR="$INST_DIR"
>      cd "$BASEDIR"
> diff --git a/defconfig b/defconfig
> index 4ec8a0b..283e954 100644
> --- a/defconfig
> +++ b/defconfig
> @@ -1,9 +1,9 @@
>  # Config variables for raisin
>  
>  # Components
> -# All components: xen grub libvirt
> -# Core xen functionality: xen
> -DEFAULT_COMPONENTS="xen grub libvirt"
> +# All components: blktap xen grub libvirt
> +# Core xen functionality: xen blktap
> +DEFAULT_COMPONENTS="blktap xen grub libvirt"
>  
>  # Build config
>  ## Make command to run
> @@ -22,6 +22,8 @@ XEN_URL="git://xenbits.xen.org/xen.git"
>  QEMU_URL="git://git.qemu.org/qemu.git"
>  GRUB_URL="git://git.savannah.gnu.org/grub.git"
>  LIBVIRT_URL="git://libvirt.org/libvirt.git"
> +BLKTAP_URL=https://github.com/xapi-project/blktap.git
> +#BLKTAP_URL=git://xenbits.xen.org/blktap.git
>  
>  # Software versions. Leave blank if you want to avoid the build, like
>  # this: GRUB_REVISION=
> @@ -30,3 +32,4 @@ XEN_REVISION="master"
>  QEMU_REVISION="master"
>  GRUB_REVISION="master"
>  LIBVIRT_REVISION="master"
> +BLKTAP_REVISION="blktap2"
> -- 
> 1.9.1
> 

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Ian Campbell]

On Fri, 2015-04-17 at 11:50 +0100, Stefano Stabellini wrote:

> > +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
> > +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
> 
> Redhat? I don't know no Redhat.

Is the use of x86_32 rather than common on the x86_64 case deliberate?

Ian.

Re: [PATCH 1/8] raisin: Fix non-verbose case

[George Dunlap]

On 04/17/2015 11:06 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> Because we use "set -e", we can't use the "a && b" construct, as it will fail and stop the script.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> I am wondering whether we should ban both a && b and a || b from the
> code and just go with the more verbose but also harder to get wrong:
> 
> if [[ $VERBOSE -eq 1 ]]
> then
>     echo calling "$component"_"$1"
> fi
> 
> Reading the code and seeing a || b will tempt you into writing a && b.

That's a good point. :-)  Do you want to go it or shall I resend?

 -George

Re: [PATCH 3/8] raisin: Use fakeroot for mkdeb so we can build the package as non-root

[George Dunlap]

On 04/17/2015 09:43 AM, Ian Campbell wrote:
> On Thu, 2015-04-16 at 17:19 +0100, George Dunlap wrote:
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> AIUI raisin does build dependency handling/tracking. If so then fakeroot
> ought to be added to it I think?

The Ubuntu docker image seems to include it (or it's already implied by
other dependencies); but yes, I'll add that.


>> ---
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> Does CC below a CC work? Interesting.

CC below a --- line you mean?

 -George

Re: [PATCH 5/8] raisin: Fix CentOS build

[George Dunlap]

On 04/17/2015 11:14 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> Add package dependencies for CentOS.  Also use PKGTYPE rather than
>> DISTRO to determine if we need rpm-build.
>>
>> I've tested this for xen but not for libvirt or grub.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  components/grub    |  5 +++++
>>  components/libvirt |  7 +++++++
>>  components/xen     | 10 ++++++++--
>>  lib/commands.sh    |  2 +-
>>  4 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/components/grub b/components/grub
>> index 563a28c..af396d9 100644
>> --- a/components/grub
>> +++ b/components/grub
>> @@ -12,6 +12,11 @@ function grub_check_package() {
>>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
>>      local DEP_Fedora_x86_64="$DEP_Fedora_common glibc-devel.i686"
>>  
>> +    local DEP_CentOS_common="make gcc tar automake autoconf sysconftool bison flex \
>> +                             glibc-devel"
>> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
>> +    local DEP_CentOS_x86_64="$DEP_CentOS_common glibc-devel.i686"
> 
> Given that they are the same as Fedora, I think it is OK to:
> 
> local DEP_CentOS_common="$DEP_Fedora_common"

In a previous version of the patch I had
"DEP_RedHat_{common,x86_32,x86_64}" (to mean things that were common
between all RH decendants, like Fedora or CentOS); but I couldn't make
the include stuff work properly.  Maybe just have a DEP_RedHat_common,
and allow the x86-specific ones to include it?

>>  
>>      if [[ $ARCH != "x86_64" && $ARCH != "x86_32" ]]
>>      then
>> diff --git a/components/libvirt b/components/libvirt
>> index 5853950..aef1bc8 100644
>> --- a/components/libvirt
>> +++ b/components/libvirt
>> @@ -18,6 +18,13 @@ function libvirt_check_package() {
>>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
>>      local DEP_Fedora_x86_64="$DEP_Fedora_common"
>>  
>> +    local DEP_CentOS_common="patch make gcc libtool autoconf gettext-devel     \
>> +                             python-devel libxslt yajl-devel libxml2-devel     \
>> +                             device-mapper-devel libpciaccess-devel            \
>> +                             libuuid-devel perl-XML-XPath"
>> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
>> +    local DEP_CentOS_x86_64="$DEP_CentOS_common"
> 
> Same here, also please test the libvirt build: the list of dependencies
> is pretty big, I worry that one of them might actually differ from Fedora

If there were something missing, it wouldn't be a regression (since
libvirt doesn't apply without this patch either).  Testing libvirt is on
my to-do list, but if I don't get to it, would you mind checking it in
as-is (once the series is in better shape)?  I'll definitely get libvirt
working before the release.

 -George

Re: [PATCH 6/8] raisin: Break build into components and allow the sub-steps to be called individually

[George Dunlap]

On 04/17/2015 11:18 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> This makes it easier to debug just one aspect of the build.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  lib/commands.sh | 16 ++++++++++++----
>>  raise           |  2 +-
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/commands.sh b/lib/commands.sh
>> index 17e2129..c5df9ad 100755
>> --- a/lib/commands.sh
>> +++ b/lib/commands.sh
>> @@ -45,17 +45,25 @@ function install-builddep() {
>>      YES=y check-builddep
>>  }
>>  
>> -function build() {
>> -    check-builddep
>> -    
>> +function build-only() {
>>      mkdir -p "$INST_DIR" &>/dev/null
>>  
>>      # build and install under $DESTDIR
>>      for_each_component build
>> -    
>> +}
>> +
>> +function build-package() {
>>      build_package xen-system
>>  }
>>  
>> +function build() {
>> +    check-builddep
>> +
>> +    build-only
>> +
>> +    build-package
>> +}
>> +
>>  function unraise() {
>>      for_each_component clean
>>  
>> diff --git a/raise b/raise
>> index bce6908..59de04a 100755
>> --- a/raise
>> +++ b/raise
>> @@ -64,7 +64,7 @@ do
>>  done
>>  
>>  case "$1" in
>> -    "install-builddep" | "build" | "install" | "configure" | "unraise" )
>> +    "install-builddep" | "build" | "build-only" | "build-package" | "install" | "configure" | "unraise" )
>>          COMMAND=$1
>>          ;;
>>      *)
> 
> You have added two new commands, but they are not documented in the help
> function or in the README. As they stand they are "hidden" commands. Is
> that on purpose because they are supposed to be used just for debugging?
> Maybe it would be worth documenting debugging functions too somewhere.

No, they were meant to be visible.  I'll document them.

 -George

Re: [PATCH 3/8] raisin: Use fakeroot for mkdeb so we can build the package as non-root

[Ian Campbell]

On Mon, 2015-04-20 at 09:57 +0100, George Dunlap wrote:
> On 04/17/2015 09:43 AM, Ian Campbell wrote:
> > On Thu, 2015-04-16 at 17:19 +0100, George Dunlap wrote:
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > 
> > AIUI raisin does build dependency handling/tracking. If so then fakeroot
> > ought to be added to it I think?
> 
> The Ubuntu docker image seems to include it (or it's already implied by
> other dependencies); but yes, I'll add that.

I can't see which dep it would be, but regardless: thanks.

> >> ---
> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > 
> > Does CC below a CC work? Interesting.
> 
> CC below a --- line you mean?

Yes. Clearly my fingers were faster than my brain last week.

Ian.

Re: [PATCH 7/8] raisin: Rework component specification

[George Dunlap]

On 04/17/2015 11:37 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> Allow COMPONENTS to be specified in the config (or on the command-line)
>>
>> Now you can keep all components enabled in your config but build only
>> one like so:
>>
>> COMPONENTS="xen" ./raise build
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> Good idea, I wanted to fix the way we disable components for a while now
> 
> 
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  defconfig               |  5 +++++
>>  lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
>>  2 files changed, 40 insertions(+), 14 deletions(-)
>>
>> diff --git a/defconfig b/defconfig
>> index 23c76eb..4ec8a0b 100644
>> --- a/defconfig
>> +++ b/defconfig
>> @@ -1,5 +1,10 @@
>>  # Config variables for raisin
>>  
>> +# Components
>> +# All components: xen grub libvirt
>> +# Core xen functionality: xen
>> +DEFAULT_COMPONENTS="xen grub libvirt"
> 
> I would just call this COMPONENTS, also see below.
> Also please update the other comment in this file on how to disable components.

I think the problem here is that if you just say "COMPONENTS=" in the
config file, then it will override the COMPONENTS= on the command-line
(i.e., 'COMPONENTS="xen" ./raise build' no longer works as expected).

We could use one of the "set if not set already" bashisms, but I think
those are all pretty ugly, and probably easy to accidentally forget when
editing.  Since the config file is part of the public interface, I
thought it important to keep it simple and clean.

>>  # Build config
>>  ## Make command to run
>>  MAKE="make -j2"
>> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
>> index e66c6f4..42406e9 100644
>> --- a/lib/common-functions.sh
>> +++ b/lib/common-functions.sh
>> @@ -31,13 +31,41 @@ function common_init() {
>>  
>>      get_distro
>>      get_arch
>> +    get_components
>>  
>> -    for f in `cat "$BASEDIR"/components/series`
>> +    echo "Distro: $DISTRO"
>> +    echo "Arch: $ARCH"
>> +    echo "Components: $COMPONENTS"
> 
> if [[ $VERBOSE -eq 1 ]]
> then
>     echo stuff
> fi
> 
> To make the code nicer to read, we could have a function called
> verbose_echo:
> 
> function verbose_echo() {
>     if [[ $VERBOSE -eq 1 ]]
>     then
>         echo $*
>     fi
> }

I'll do the 'if' first, and maybe look at making a function later.

>> +function get_components() {
>> +    if [[ -z "$COMPONENTS" ]]
>> +    then
>> +	COMPONENTS="$DEFAULT_COMPONENTS"
> 
> Stray tab.

Weird... I thought I had emacs set to use no tabs.  I'll take a look...

> I would call our internal components variable RAISIN_COMPONENTS and the
> one in the config and exposed to users COMPONENTS.

Any revision to this given my answer above?  Off the top of my head I
don't see a reason to have three variables.

 -George

Re: [PATCH 7/8] raisin: Rework component specification

[George Dunlap]

On 04/17/2015 11:40 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> Allow COMPONENTS to be specified in the config (or on the command-line)
>>
>> Now you can keep all components enabled in your config but build only
>> one like so:
>>
>> COMPONENTS="xen" ./raise build
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  defconfig               |  5 +++++
>>  lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
>>  2 files changed, 40 insertions(+), 14 deletions(-)
>>
>> diff --git a/defconfig b/defconfig
>> index 23c76eb..4ec8a0b 100644
>> --- a/defconfig
>> +++ b/defconfig
>> @@ -1,5 +1,10 @@
>>  # Config variables for raisin
>>  
>> +# Components
>> +# All components: xen grub libvirt
>> +# Core xen functionality: xen
>> +DEFAULT_COMPONENTS="xen grub libvirt"
>> +
>>  # Build config
>>  ## Make command to run
>>  MAKE="make -j2"
>> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
>> index e66c6f4..42406e9 100644
>> --- a/lib/common-functions.sh
>> +++ b/lib/common-functions.sh
>> @@ -31,13 +31,41 @@ function common_init() {
>>  
>>      get_distro
>>      get_arch
>> +    get_components
>>  
>> -    for f in `cat "$BASEDIR"/components/series`
>> +    echo "Distro: $DISTRO"
>> +    echo "Arch: $ARCH"
>> +    echo "Components: $COMPONENTS"
>> +
>> +    for f in $COMPONENTS
>>      do
>>          source "$BASEDIR"/components/"$f"
>>      done
>>  }
>>  
>> +function get_components() {
>> +    if [[ -z "$COMPONENTS" ]]
>> +    then
>> +	COMPONENTS="$DEFAULT_COMPONENTS"
>> +    fi
>> +
>> +    if [[ -z "$COMPONENTS" ]] 
>> +    then 
>> +	local component
>> +	for component in `cat "$BASEDIR"/components/series`
>> +	do
>> +	    local capital
>> +            capital=`echo $component | tr '[:lower:]' '[:upper:]'`
>> +            if eval [[ ! -z \$"$capital"_UPSTREAM_REVISION ]]
>> +            then
>> +		COMPONENTS="$COMPONENTS $component"
>> +		echo "Found component $component"
>> +            fi
>> +	done
>> +	export COMPONENTS
>> +    fi
>> +}
>> +
>>  function get_distro() {
>>      if [[ -x "`which lsb_release 2>/dev/null`" ]]
>>      then
>> @@ -222,20 +250,13 @@ function stop_initscripts() {
>>  }
>>  
>>  function for_each_component () {
>> -    for component in `cat "$BASEDIR"/components/series`
>> +    local component
> 
> One important thing here is that the order is important: xen is the
> first item in the series file for a reason. I think we should use the
> COMPONENTS variable to check for what components are enabled, but then
> still use the series file for the ordering.

Yes, I thought about that.  I had the idea it wouldn't be the easiest
thing to do in bash, but I'll take a look.

FYI I basically spent the "development day" for 4 weeks on those two
series, so it may be a while before I get back to it. :-)  (Depends on
how much review there is to do.)

 -George

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

On 04/17/2015 11:50 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> Until we start hosting the blktap repo on xenbits, use the one from github.
>>
>> Also, we need to pass in the directories where to find the
>> libblktapctl headers in the Xen build.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>> Note: For now use the "upstream" XenServer repo.
>>
>> Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
>> which can be found here:
>>
>> git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2
>>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  components/series |  1 +
>>  components/xen    |  5 +++++
>>  defconfig         |  9 ++++++---
>>  4 files changed, 59 insertions(+), 3 deletions(-)
>>  create mode 100644 components/blktap
>>
>> diff --git a/components/blktap b/components/blktap
>> new file mode 100644
>> index 0000000..3062b2a
>> --- /dev/null
>> +++ b/components/blktap
>> @@ -0,0 +1,47 @@
>> +#!/usr/bin/env bash
>> +
>> +function blktap_check_package() {
>> +    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
>> +    local DEP_Debian_x86_32="$DEP_Debian_common"
>> +    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
>> +    local DEP_Debian_arm32="$DEP_Debian_common"
>> +    local DEP_Debian_arm64="$DEP_Debian_arm32"
>> +
>> +    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
>> +    local DEP_Fedora_x86_32="$DEP_Fedora_common"
>> +    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
>> +
>> +    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> 
> DEP_CentOS_common="$DEP_Fedora_common"
> 
> 
>> +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
>> +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
> 
> Redhat? I don't know no Redhat.

Sorry, artifacts from a previous version of the patch...
>> +    fi
>> +}
>> +
>> +function blktap_configure() {
>> +    :
>> +}
>> +
>> +function blktap_unconfigure() {
>> +    :
>> +}
> 
> Much better than my echo. Maybe submit a patch to replace useless echo
> in configure and unconfigure functions with :

I can put it on my to-do list, but it may be a while before it reaches
the top. :-)

>> diff --git a/components/series b/components/series
>> index 8f614f0..953800a 100644
>> --- a/components/series
>> +++ b/components/series
>> @@ -1,3 +1,4 @@
>> +blktap
>>  xen
> 
> I take this is on purpose so that blktap is built before xen

Yes, as Xen has to link against the libraries.

>>  qemu
>>  grub
>> diff --git a/components/xen b/components/xen
>> index f2e1254..03a1970 100644
>> --- a/components/xen
>> +++ b/components/xen
>> @@ -29,7 +29,12 @@ function xen_build() {
>>      cd "$BASEDIR"
>>      git-checkout $XEN_URL $XEN_REVISION xen-dir
>>      cd xen-dir
>> +    CFLAGS="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
>> +    LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
>>      ./configure --prefix=$PREFIX --with-system-qemu=/usr/bin/qemu-system-i386
>> +    CFLAGS_libblktapctl="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
>> +    LDLIBS_libblktapctl="-L$INST_DIR/$PREFIX/lib -lblktapctl" \
>> +    SHLIB_libblktapctl="-Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
> 
> Does this work even if blktap is actually disabled in the raisin build?
> If so, then fine.

I don't think I've tested it, but in theory yes it should work.  If
blktap support isn't specified, the patch series I submitted to
xen-unstable will automatically enable blktap if found and disable it if
not found.  So if you have a clean raisin repo and just build the xen
component, it should build it without blktap.

(With my patches,  if you specify --enable-blktap and it can't find it,
config will throw an error.  But we're not enabling it.)

> Actually I think that the xen configure script should be able to figure
> out whether it can pick up blktap from an external location
> automatically with the CFLAGS and LDFLAGS you set.

Actually, what would be better is if you could just set
"-I$INST_DIR/$PREFIX/include" and "-L$INST_DIR/$PREFIX/lib" and be done
with it.  But the blktap2 headers are broken at the moment: they're
placed in subdirectories on install time that don't reflect the
directory structure used when building, so if you don't specify both
subdirectories explicitly, it won't compile.

That's something I plan to fix, but it may take a bit to get back to it.

 -George

Re: [PATCH 1/8] raisin: Fix non-verbose case

[Stefano Stabellini]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:06 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Because we use "set -e", we can't use the "a && b" construct, as it will fail and stop the script.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > 
> > I am wondering whether we should ban both a && b and a || b from the
> > code and just go with the more verbose but also harder to get wrong:
> > 
> > if [[ $VERBOSE -eq 1 ]]
> > then
> >     echo calling "$component"_"$1"
> > fi
> > 
> > Reading the code and seeing a || b will tempt you into writing a && b.
> 
> That's a good point. :-)  Do you want to go it or shall I resend?

Given that you have a couple of other patches to resend, you might as
well resend this one too.

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:50 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Until we start hosting the blktap repo on xenbits, use the one from github.
> >>
> >> Also, we need to pass in the directories where to find the
> >> libblktapctl headers in the Xen build.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >> ---
> >> Note: For now use the "upstream" XenServer repo.
> >>
> >> Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
> >> which can be found here:
> >>
> >> git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2
> >>
> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >> ---
> >>  components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  components/series |  1 +
> >>  components/xen    |  5 +++++
> >>  defconfig         |  9 ++++++---
> >>  4 files changed, 59 insertions(+), 3 deletions(-)
> >>  create mode 100644 components/blktap
> >>
> >> diff --git a/components/blktap b/components/blktap
> >> new file mode 100644
> >> index 0000000..3062b2a
> >> --- /dev/null
> >> +++ b/components/blktap
> >> @@ -0,0 +1,47 @@
> >> +#!/usr/bin/env bash
> >> +
> >> +function blktap_check_package() {
> >> +    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
> >> +    local DEP_Debian_x86_32="$DEP_Debian_common"
> >> +    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
> >> +    local DEP_Debian_arm32="$DEP_Debian_common"
> >> +    local DEP_Debian_arm64="$DEP_Debian_arm32"
> >> +
> >> +    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> >> +    local DEP_Fedora_x86_32="$DEP_Fedora_common"
> >> +    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
> >> +
> >> +    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> > 
> > DEP_CentOS_common="$DEP_Fedora_common"
> > 
> > 
> >> +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
> >> +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
> > 
> > Redhat? I don't know no Redhat.
> 
> Sorry, artifacts from a previous version of the patch...
> >> +    fi
> >> +}
> >> +
> >> +function blktap_configure() {
> >> +    :
> >> +}
> >> +
> >> +function blktap_unconfigure() {
> >> +    :
> >> +}
> > 
> > Much better than my echo. Maybe submit a patch to replace useless echo
> > in configure and unconfigure functions with :
> 
> I can put it on my to-do list, but it may be a while before it reaches
> the top. :-)
> 
> >> diff --git a/components/series b/components/series
> >> index 8f614f0..953800a 100644
> >> --- a/components/series
> >> +++ b/components/series
> >> @@ -1,3 +1,4 @@
> >> +blktap
> >>  xen
> > 
> > I take this is on purpose so that blktap is built before xen
> 
> Yes, as Xen has to link against the libraries.
> 
> >>  qemu
> >>  grub
> >> diff --git a/components/xen b/components/xen
> >> index f2e1254..03a1970 100644
> >> --- a/components/xen
> >> +++ b/components/xen
> >> @@ -29,7 +29,12 @@ function xen_build() {
> >>      cd "$BASEDIR"
> >>      git-checkout $XEN_URL $XEN_REVISION xen-dir
> >>      cd xen-dir
> >> +    CFLAGS="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
> >> +    LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
> >>      ./configure --prefix=$PREFIX --with-system-qemu=/usr/bin/qemu-system-i386
> >> +    CFLAGS_libblktapctl="-I$INST_DIR/$PREFIX/include/blktap -I$INST_DIR/$PREFIX/include/vhd" \
> >> +    LDLIBS_libblktapctl="-L$INST_DIR/$PREFIX/lib -lblktapctl" \
> >> +    SHLIB_libblktapctl="-Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
> > 
> > Does this work even if blktap is actually disabled in the raisin build?
> > If so, then fine.
> 
> I don't think I've tested it, but in theory yes it should work.  If
> blktap support isn't specified, the patch series I submitted to
> xen-unstable will automatically enable blktap if found and disable it if
> not found.  So if you have a clean raisin repo and just build the xen
> component, it should build it without blktap.
> 
> (With my patches,  if you specify --enable-blktap and it can't find it,
> config will throw an error.  But we're not enabling it.)

If you test it and it works both with and without blktap, then I am OK
with it as is.


> > Actually I think that the xen configure script should be able to figure
> > out whether it can pick up blktap from an external location
> > automatically with the CFLAGS and LDFLAGS you set.
> 
> Actually, what would be better is if you could just set
> "-I$INST_DIR/$PREFIX/include" and "-L$INST_DIR/$PREFIX/lib" and be done
> with it.  But the blktap2 headers are broken at the moment: they're
> placed in subdirectories on install time that don't reflect the
> directory structure used when building, so if you don't specify both
> subdirectories explicitly, it won't compile.
> 
> That's something I plan to fix, but it may take a bit to get back to it.
> 
>  -George
> 

Re: [PATCH 7/8] raisin: Rework component specification

[Stefano Stabellini]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:37 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Allow COMPONENTS to be specified in the config (or on the command-line)
> >>
> >> Now you can keep all components enabled in your config but build only
> >> one like so:
> >>
> >> COMPONENTS="xen" ./raise build
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > 
> > Good idea, I wanted to fix the way we disable components for a while now
> > 
> > 
> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >> ---
> >>  defconfig               |  5 +++++
> >>  lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
> >>  2 files changed, 40 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/defconfig b/defconfig
> >> index 23c76eb..4ec8a0b 100644
> >> --- a/defconfig
> >> +++ b/defconfig
> >> @@ -1,5 +1,10 @@
> >>  # Config variables for raisin
> >>  
> >> +# Components
> >> +# All components: xen grub libvirt
> >> +# Core xen functionality: xen
> >> +DEFAULT_COMPONENTS="xen grub libvirt"
> > 
> > I would just call this COMPONENTS, also see below.
> > Also please update the other comment in this file on how to disable components.
> 
> I think the problem here is that if you just say "COMPONENTS=" in the
> config file, then it will override the COMPONENTS= on the command-line
> (i.e., 'COMPONENTS="xen" ./raise build' no longer works as expected).
> 
> We could use one of the "set if not set already" bashisms, but I think
> those are all pretty ugly, and probably easy to accidentally forget when
> editing.  Since the config file is part of the public interface, I
> thought it important to keep it simple and clean.

You are right, it is important to keep the config file simple and clean,
that is why I was suggesting to use a simpler name for the config
variable. I certainly wouldn't want any of the "set if not set already"
constructs in the config file.

I guess that having COMPONENTS="xen" ./raise build would be nice and it
is simpler to achieve it by having a different name for the variable in
the config file.

OK, you convinced me :-)




> >>  # Build config
> >>  ## Make command to run
> >>  MAKE="make -j2"
> >> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> >> index e66c6f4..42406e9 100644
> >> --- a/lib/common-functions.sh
> >> +++ b/lib/common-functions.sh
> >> @@ -31,13 +31,41 @@ function common_init() {
> >>  
> >>      get_distro
> >>      get_arch
> >> +    get_components
> >>  
> >> -    for f in `cat "$BASEDIR"/components/series`
> >> +    echo "Distro: $DISTRO"
> >> +    echo "Arch: $ARCH"
> >> +    echo "Components: $COMPONENTS"
> > 
> > if [[ $VERBOSE -eq 1 ]]
> > then
> >     echo stuff
> > fi
> > 
> > To make the code nicer to read, we could have a function called
> > verbose_echo:
> > 
> > function verbose_echo() {
> >     if [[ $VERBOSE -eq 1 ]]
> >     then
> >         echo $*
> >     fi
> > }
> 
> I'll do the 'if' first, and maybe look at making a function later.
> 
> >> +function get_components() {
> >> +    if [[ -z "$COMPONENTS" ]]
> >> +    then
> >> +	COMPONENTS="$DEFAULT_COMPONENTS"
> > 
> > Stray tab.
> 
> Weird... I thought I had emacs set to use no tabs.  I'll take a look...
> 
> > I would call our internal components variable RAISIN_COMPONENTS and the
> > one in the config and exposed to users COMPONENTS.
> 
> Any revision to this given my answer above?  Off the top of my head I
> don't see a reason to have three variables.
> 
>  -George
> 

Re: [PATCH 5/8] raisin: Fix CentOS build

[Stefano Stabellini]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:14 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Add package dependencies for CentOS.  Also use PKGTYPE rather than
> >> DISTRO to determine if we need rpm-build.
> >>
> >> I've tested this for xen but not for libvirt or grub.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >> ---
> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >> ---
> >>  components/grub    |  5 +++++
> >>  components/libvirt |  7 +++++++
> >>  components/xen     | 10 ++++++++--
> >>  lib/commands.sh    |  2 +-
> >>  4 files changed, 21 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/components/grub b/components/grub
> >> index 563a28c..af396d9 100644
> >> --- a/components/grub
> >> +++ b/components/grub
> >> @@ -12,6 +12,11 @@ function grub_check_package() {
> >>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
> >>      local DEP_Fedora_x86_64="$DEP_Fedora_common glibc-devel.i686"
> >>  
> >> +    local DEP_CentOS_common="make gcc tar automake autoconf sysconftool bison flex \
> >> +                             glibc-devel"
> >> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
> >> +    local DEP_CentOS_x86_64="$DEP_CentOS_common glibc-devel.i686"
> > 
> > Given that they are the same as Fedora, I think it is OK to:
> > 
> > local DEP_CentOS_common="$DEP_Fedora_common"
> 
> In a previous version of the patch I had
> "DEP_RedHat_{common,x86_32,x86_64}" (to mean things that were common
> between all RH decendants, like Fedora or CentOS); but I couldn't make
> the include stuff work properly.  Maybe just have a DEP_RedHat_common,
> and allow the x86-specific ones to include it?

But from the look of your patch the list of dependencies at the moment
is exactly the same between Fedora and CentOS, so I would avoid
DEP_RedHat_{common,x86_32,x86_64}, I would just local
DEP_CentOS_common="$DEP_Fedora_common".


> >>  
> >>      if [[ $ARCH != "x86_64" && $ARCH != "x86_32" ]]
> >>      then
> >> diff --git a/components/libvirt b/components/libvirt
> >> index 5853950..aef1bc8 100644
> >> --- a/components/libvirt
> >> +++ b/components/libvirt
> >> @@ -18,6 +18,13 @@ function libvirt_check_package() {
> >>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
> >>      local DEP_Fedora_x86_64="$DEP_Fedora_common"
> >>  
> >> +    local DEP_CentOS_common="patch make gcc libtool autoconf gettext-devel     \
> >> +                             python-devel libxslt yajl-devel libxml2-devel     \
> >> +                             device-mapper-devel libpciaccess-devel            \
> >> +                             libuuid-devel perl-XML-XPath"
> >> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
> >> +    local DEP_CentOS_x86_64="$DEP_CentOS_common"
> > 
> > Same here, also please test the libvirt build: the list of dependencies
> > is pretty big, I worry that one of them might actually differ from Fedora
> 
> If there were something missing, it wouldn't be a regression (since
> libvirt doesn't apply without this patch either).  Testing libvirt is on
> my to-do list, but if I don't get to it, would you mind checking it in
> as-is (once the series is in better shape)?  I'll definitely get libvirt
> working before the release.

Having the code in will give the impression that it works already, so I
am not very happy about this, but OK.

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

On 04/17/2015 11:58 AM, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
>> Until we start hosting the blktap repo on xenbits, use the one from github.
>>
>> Also, we need to pass in the directories where to find the
>> libblktapctl headers in the Xen build.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>> Note: For now use the "upstream" XenServer repo.
>>
>> Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
>> which can be found here:
>>
>> git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2
>>
>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>> ---
>>  components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  components/series |  1 +
>>  components/xen    |  5 +++++
>>  defconfig         |  9 ++++++---
>>  4 files changed, 59 insertions(+), 3 deletions(-)
>>  create mode 100644 components/blktap
>>
>> diff --git a/components/blktap b/components/blktap
>> new file mode 100644
>> index 0000000..3062b2a
>> --- /dev/null
>> +++ b/components/blktap
>> @@ -0,0 +1,47 @@
>> +#!/usr/bin/env bash
>> +
>> +function blktap_check_package() {
>> +    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
>> +    local DEP_Debian_x86_32="$DEP_Debian_common"
>> +    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
>> +    local DEP_Debian_arm32="$DEP_Debian_common"
>> +    local DEP_Debian_arm64="$DEP_Debian_arm32"
>> +
>> +    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
>> +    local DEP_Fedora_x86_32="$DEP_Fedora_common"
>> +    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
>> +
>> +    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
>> +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
>> +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
>> +
>> +    echo Checking blktap dependencies
>> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
>> +}
>> +
>> +function blktap_build() {
>> +    cd "$BASEDIR"
>> +    git-checkout $BLKTAP_URL $BLKTAP_REVISION blktap-dir
>> +    cd blktap-dir
>> +    ./autogen.sh
>> +    ./configure --prefix=$PREFIX
>> +    $MAKE
>> +    $MAKE install DESTDIR="$INST_DIR"
>> +    cd "$BASEDIR"
>> +}
> 
> I think we need to disable the build on architectures other than x86,
> see grub for example

Do we?  There's no reason a blktap2 kernel module couldn't be built on
ARM, is there?

And in any case the long-term plan is to get blktap3 working as well,
which doesn't require kernel-space drivers.

Or do you mean on systems other than Linux?

 -George

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:58 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Until we start hosting the blktap repo on xenbits, use the one from github.
> >>
> >> Also, we need to pass in the directories where to find the
> >> libblktapctl headers in the Xen build.
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >> ---
> >> Note: For now use the "upstream" XenServer repo.
> >>
> >> Also, to build with this you need the patches to allow Xen to use an out-of-tree blktap,
> >> which can be found here:
> >>
> >> git://xenbits.xen.org/people/gdunlap/xen.git  out/blktap25/rfc-v2
> >>
> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >> ---
> >>  components/blktap | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  components/series |  1 +
> >>  components/xen    |  5 +++++
> >>  defconfig         |  9 ++++++---
> >>  4 files changed, 59 insertions(+), 3 deletions(-)
> >>  create mode 100644 components/blktap
> >>
> >> diff --git a/components/blktap b/components/blktap
> >> new file mode 100644
> >> index 0000000..3062b2a
> >> --- /dev/null
> >> +++ b/components/blktap
> >> @@ -0,0 +1,47 @@
> >> +#!/usr/bin/env bash
> >> +
> >> +function blktap_check_package() {
> >> +    local DEP_Debian_common="build-essential autoconf libtool tar libaio-dev uuid-dev openssl-dev"
> >> +    local DEP_Debian_x86_32="$DEP_Debian_common"
> >> +    local DEP_Debian_x86_64="$DEP_Debian_x86_32"
> >> +    local DEP_Debian_arm32="$DEP_Debian_common"
> >> +    local DEP_Debian_arm64="$DEP_Debian_arm32"
> >> +
> >> +    local DEP_Fedora_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> >> +    local DEP_Fedora_x86_32="$DEP_Fedora_common"
> >> +    local DEP_Fedora_x86_64="$DEP_Fedora_x86_32"
> >> +
> >> +    local DEP_CentOS_common="make gcc python-devel autoconf libtool tar file libaio-devel libuuid-devel openssl-devel"
> >> +    local DEP_CentOS_x86_32="$DEP_Redhat_common"
> >> +    local DEP_CentOS_x86_64="$DEP_Redhat_x86_32"
> >> +
> >> +    echo Checking blktap dependencies
> >> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
> >> +}
> >> +
> >> +function blktap_build() {
> >> +    cd "$BASEDIR"
> >> +    git-checkout $BLKTAP_URL $BLKTAP_REVISION blktap-dir
> >> +    cd blktap-dir
> >> +    ./autogen.sh
> >> +    ./configure --prefix=$PREFIX
> >> +    $MAKE
> >> +    $MAKE install DESTDIR="$INST_DIR"
> >> +    cd "$BASEDIR"
> >> +}
> > 
> > I think we need to disable the build on architectures other than x86,
> > see grub for example
> 
> Do we?  There's no reason a blktap2 kernel module couldn't be built on
> ARM, is there?

Maybe not, but I am pretty sure that it doesn't work at the moment. I
don't think that the userspace stuff even compiles on ARM.
Eventually we might have blktap on ARM, but I don't want to enable
stuff in Raisin that we know it does not work.


> And in any case the long-term plan is to get blktap3 working as well,
> which doesn't require kernel-space drivers.
> 
> Or do you mean on systems other than Linux?

Re: [PATCH 1/8] raisin: Fix non-verbose case

[Stefano Stabellini]

On Mon, 20 Apr 2015, Stefano Stabellini wrote:
> On Mon, 20 Apr 2015, George Dunlap wrote:
> > On 04/17/2015 11:06 AM, Stefano Stabellini wrote:
> > > On Thu, 16 Apr 2015, George Dunlap wrote:
> > >> Because we use "set -e", we can't use the "a && b" construct, as it will fail and stop the script.
> > >>
> > >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > > 
> > > I am wondering whether we should ban both a && b and a || b from the
> > > code and just go with the more verbose but also harder to get wrong:
> > > 
> > > if [[ $VERBOSE -eq 1 ]]
> > > then
> > >     echo calling "$component"_"$1"
> > > fi
> > > 
> > > Reading the code and seeing a || b will tempt you into writing a && b.
> > 
> > That's a good point. :-)  Do you want to go it or shall I resend?
> 
> Given that you have a couple of other patches to resend, you might as
> well resend this one too.

I made the change and committed it.

Re: [PATCH 3/8] raisin: Use fakeroot for mkdeb so we can build the package as non-root

[Stefano Stabellini]

On Fri, 17 Apr 2015, Stefano Stabellini wrote:
> On Fri, 17 Apr 2015, Ian Campbell wrote:
> > On Thu, 2015-04-16 at 17:19 +0100, George Dunlap wrote:
> > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > 
> > AIUI raisin does build dependency handling/tracking. If so then fakeroot
> > ought to be added to it I think?
> 
> Good point! Please add fakeroot as Debian specific dependency for every
> component, like rpm-build for Fedora.

I made the change and commit it.


> 
> > > ---
> > > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > 
> > Does CC below a CC work? Interesting.
> > 
> > > ---
> > >  lib/common-functions.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> > > index 06c894a..373d6fb 100644
> > > --- a/lib/common-functions.sh
> > > +++ b/lib/common-functions.sh
> > > @@ -242,7 +242,7 @@ function for_each_component () {
> > >  function build_package() {
> > >      if [[ $DISTRO = "Debian" ]]
> > >      then
> > > -        ./scripts/mkdeb "$1"
> > > +        fakeroot bash ./scripts/mkdeb "$1"
> > >      elif [[  $DISTRO = "Fedora" ]]
> > >      then
> > >          ./scripts/mkrpm "$1"
> > 
> > 
> 

Re: [PATCH 4/8] raisin: Use PKGTYPE rather than DISTRO to determine how to build a package

[Stefano Stabellini]

On Fri, 17 Apr 2015, Stefano Stabellini wrote:
> On Thu, 16 Apr 2015, George Dunlap wrote:
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > ---
> > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > ---
> >  lib/common-functions.sh | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> > index 373d6fb..e66c6f4 100644
> > --- a/lib/common-functions.sh
> > +++ b/lib/common-functions.sh
> > @@ -239,16 +239,16 @@ function for_each_component () {
> >      done
> >  }
> >  
> > +function _build_package_deb() {
> > +    fakeroot bash ./scripts/mkdeb "$1"
> > +}
> > +
> > +function _build_package_rpm() {
> > +    ./scripts/mkrpm "$1"
> > +}
> > +
> >  function build_package() {
> > -    if [[ $DISTRO = "Debian" ]]
> > -    then
> > -        fakeroot bash ./scripts/mkdeb "$1"
> > -    elif [[  $DISTRO = "Fedora" ]]
> > -    then
> > -        ./scripts/mkrpm "$1"
> > -    else
> > -        echo "Don't know how to create packages for $DISTRO"
> > -    fi
> > +    _build_package_${PKGTYPE} "$1"
> 
> Just use "$PKGTYPE" instead of ${}
> Also please document PKGTYPE among the exported global variables in the
> README.

I made the changes and committed.


> 
> >  }
> >  
> >  function install_package() {
> > -- 
> > 1.9.1
> > 
> 

Re: [PATCH 5/8] raisin: Fix CentOS build

[Stefano Stabellini]

On Mon, 20 Apr 2015, Stefano Stabellini wrote:
> On Mon, 20 Apr 2015, George Dunlap wrote:
> > On 04/17/2015 11:14 AM, Stefano Stabellini wrote:
> > > On Thu, 16 Apr 2015, George Dunlap wrote:
> > >> Add package dependencies for CentOS.  Also use PKGTYPE rather than
> > >> DISTRO to determine if we need rpm-build.
> > >>
> > >> I've tested this for xen but not for libvirt or grub.
> > >>
> > >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > >> ---
> > >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> > >> ---
> > >>  components/grub    |  5 +++++
> > >>  components/libvirt |  7 +++++++
> > >>  components/xen     | 10 ++++++++--
> > >>  lib/commands.sh    |  2 +-
> > >>  4 files changed, 21 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/components/grub b/components/grub
> > >> index 563a28c..af396d9 100644
> > >> --- a/components/grub
> > >> +++ b/components/grub
> > >> @@ -12,6 +12,11 @@ function grub_check_package() {
> > >>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
> > >>      local DEP_Fedora_x86_64="$DEP_Fedora_common glibc-devel.i686"
> > >>  
> > >> +    local DEP_CentOS_common="make gcc tar automake autoconf sysconftool bison flex \
> > >> +                             glibc-devel"
> > >> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
> > >> +    local DEP_CentOS_x86_64="$DEP_CentOS_common glibc-devel.i686"
> > > 
> > > Given that they are the same as Fedora, I think it is OK to:
> > > 
> > > local DEP_CentOS_common="$DEP_Fedora_common"
> > 
> > In a previous version of the patch I had
> > "DEP_RedHat_{common,x86_32,x86_64}" (to mean things that were common
> > between all RH decendants, like Fedora or CentOS); but I couldn't make
> > the include stuff work properly.  Maybe just have a DEP_RedHat_common,
> > and allow the x86-specific ones to include it?
> 
> But from the look of your patch the list of dependencies at the moment
> is exactly the same between Fedora and CentOS, so I would avoid
> DEP_RedHat_{common,x86_32,x86_64}, I would just local
> DEP_CentOS_common="$DEP_Fedora_common".
> 
> 
> > >>  
> > >>      if [[ $ARCH != "x86_64" && $ARCH != "x86_32" ]]
> > >>      then
> > >> diff --git a/components/libvirt b/components/libvirt
> > >> index 5853950..aef1bc8 100644
> > >> --- a/components/libvirt
> > >> +++ b/components/libvirt
> > >> @@ -18,6 +18,13 @@ function libvirt_check_package() {
> > >>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
> > >>      local DEP_Fedora_x86_64="$DEP_Fedora_common"
> > >>  
> > >> +    local DEP_CentOS_common="patch make gcc libtool autoconf gettext-devel     \
> > >> +                             python-devel libxslt yajl-devel libxml2-devel     \
> > >> +                             device-mapper-devel libpciaccess-devel            \
> > >> +                             libuuid-devel perl-XML-XPath"
> > >> +    local DEP_CentOS_x86_32="$DEP_CentOS_common"
> > >> +    local DEP_CentOS_x86_64="$DEP_CentOS_common"
> > > 
> > > Same here, also please test the libvirt build: the list of dependencies
> > > is pretty big, I worry that one of them might actually differ from Fedora
> > 
> > If there were something missing, it wouldn't be a regression (since
> > libvirt doesn't apply without this patch either).  Testing libvirt is on
> > my to-do list, but if I don't get to it, would you mind checking it in
> > as-is (once the series is in better shape)?  I'll definitely get libvirt
> > working before the release.
> 
> Having the code in will give the impression that it works already, so I
> am not very happy about this, but OK.

I made the changes and committed.

Re: [PATCH 7/8] raisin: Rework component specification

[Stefano Stabellini]

On Mon, 20 Apr 2015, George Dunlap wrote:
> On 04/17/2015 11:40 AM, Stefano Stabellini wrote:
> > On Thu, 16 Apr 2015, George Dunlap wrote:
> >> Allow COMPONENTS to be specified in the config (or on the command-line)
> >>
> >> Now you can keep all components enabled in your config but build only
> >> one like so:
> >>
> >> COMPONENTS="xen" ./raise build
> >>
> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> >> ---
> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> >> ---
> >>  defconfig               |  5 +++++
> >>  lib/common-functions.sh | 49 +++++++++++++++++++++++++++++++++++--------------
> >>  2 files changed, 40 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/defconfig b/defconfig
> >> index 23c76eb..4ec8a0b 100644
> >> --- a/defconfig
> >> +++ b/defconfig
> >> @@ -1,5 +1,10 @@
> >>  # Config variables for raisin
> >>  
> >> +# Components
> >> +# All components: xen grub libvirt
> >> +# Core xen functionality: xen
> >> +DEFAULT_COMPONENTS="xen grub libvirt"
> >> +
> >>  # Build config
> >>  ## Make command to run
> >>  MAKE="make -j2"
> >> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> >> index e66c6f4..42406e9 100644
> >> --- a/lib/common-functions.sh
> >> +++ b/lib/common-functions.sh
> >> @@ -31,13 +31,41 @@ function common_init() {
> >>  
> >>      get_distro
> >>      get_arch
> >> +    get_components
> >>  
> >> -    for f in `cat "$BASEDIR"/components/series`
> >> +    echo "Distro: $DISTRO"
> >> +    echo "Arch: $ARCH"
> >> +    echo "Components: $COMPONENTS"
> >> +
> >> +    for f in $COMPONENTS
> >>      do
> >>          source "$BASEDIR"/components/"$f"
> >>      done
> >>  }
> >>  
> >> +function get_components() {
> >> +    if [[ -z "$COMPONENTS" ]]
> >> +    then
> >> +	COMPONENTS="$DEFAULT_COMPONENTS"
> >> +    fi
> >> +
> >> +    if [[ -z "$COMPONENTS" ]] 
> >> +    then 
> >> +	local component
> >> +	for component in `cat "$BASEDIR"/components/series`
> >> +	do
> >> +	    local capital
> >> +            capital=`echo $component | tr '[:lower:]' '[:upper:]'`
> >> +            if eval [[ ! -z \$"$capital"_UPSTREAM_REVISION ]]
> >> +            then
> >> +		COMPONENTS="$COMPONENTS $component"
> >> +		echo "Found component $component"
> >> +            fi
> >> +	done
> >> +	export COMPONENTS
> >> +    fi
> >> +}
> >> +
> >>  function get_distro() {
> >>      if [[ -x "`which lsb_release 2>/dev/null`" ]]
> >>      then
> >> @@ -222,20 +250,13 @@ function stop_initscripts() {
> >>  }
> >>  
> >>  function for_each_component () {
> >> -    for component in `cat "$BASEDIR"/components/series`
> >> +    local component
> > 
> > One important thing here is that the order is important: xen is the
> > first item in the series file for a reason. I think we should use the
> > COMPONENTS variable to check for what components are enabled, but then
> > still use the series file for the ordering.
> 
> Yes, I thought about that.  I had the idea it wouldn't be the easiest
> thing to do in bash, but I'll take a look.
> 
> FYI I basically spent the "development day" for 4 weeks on those two
> series, so it may be a while before I get back to it. :-)  (Depends on
> how much review there is to do.)

OK. I renamed DEFAULT_COMPONENTS to ENABLED_COMPONENTS, reworked this
loop, added documentation and committed the result.

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Ian Campbell]

On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
> > > I think we need to disable the build on architectures other than x86,
> > > see grub for example

Eventually we might want to build our own grub on ARM in order to pick
up Fu Wei's multiboot for arm64 patches, until they enter distros?

Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
currently works even on x86.

> > Do we?  There's no reason a blktap2 kernel module couldn't be built on
> > ARM, is there?
> 
> Maybe not, but I am pretty sure that it doesn't work at the moment. I
> don't think that the userspace stuff even compiles on ARM.
> Eventually we might have blktap on ARM, but I don't want to enable
> stuff in Raisin that we know it does not work.

Especially if it is already to a greater or lesser extent deprecated (in
favour of eventual blktap3) even on x86.

> > And in any case the long-term plan is to get blktap3 working as well,
> > which doesn't require kernel-space drivers.
> > 
> > Or do you mean on systems other than Linux?
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

On 04/21/2015 10:25 AM, Ian Campbell wrote:
> On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
>>>> I think we need to disable the build on architectures other than x86,
>>>> see grub for example
> 
> Eventually we might want to build our own grub on ARM in order to pick
> up Fu Wei's multiboot for arm64 patches, until they enter distros?
> 
> Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
> directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
> currently works even on x86.
> 
>>> Do we?  There's no reason a blktap2 kernel module couldn't be built on
>>> ARM, is there?
>>
>> Maybe not, but I am pretty sure that it doesn't work at the moment. I
>> don't think that the userspace stuff even compiles on ARM.
>> Eventually we might have blktap on ARM, but I don't want to enable
>> stuff in Raisin that we know it does not work.
> 
> Especially if it is already to a greater or lesser extent deprecated (in
> favour of eventual blktap3) even on x86.

So from my discussions w/ the XenServer guys, it seems that:

1. The "master" branch of the blktap.git repo contains support for
*both* blktap3 and blktap2.5 (with a kernel module)

2. XenServer uses blktap3 for guest access, but still use the blktap2.5
w/ kernel module for dom0 access to guest disks, to avoid the
possibility of hitting a scalability limit due to grant references.

So from raisin's perspective, the only difference between blktap2.5 and
blktap3 is using the "master" branch rather than the "blktap2" branch of
the repo.

Whether we maintain support for blktap2.5 in libxl is a matter for the
Xen maintainers; but if xapi is ever going to start using libxl, it will
certainly need to be able to do so.

(Dave / David, please correct me if I'm wrong.)

That said, there's no harm in disabling it on ARM to begin with, and
enabling it once blktap3 works.

 -George

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Tue, 21 Apr 2015, Ian Campbell wrote:
> On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
> > > > I think we need to disable the build on architectures other than x86,
> > > > see grub for example
> 
> Eventually we might want to build our own grub on ARM in order to pick
> up Fu Wei's multiboot for arm64 patches, until they enter distros?
> 
> Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
> directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
> currently works even on x86.

Both approaches are certainly possible.

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Tue, 21 Apr 2015, George Dunlap wrote:
> On 04/21/2015 10:25 AM, Ian Campbell wrote:
> > On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
> >>>> I think we need to disable the build on architectures other than x86,
> >>>> see grub for example
> > 
> > Eventually we might want to build our own grub on ARM in order to pick
> > up Fu Wei's multiboot for arm64 patches, until they enter distros?
> > 
> > Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
> > directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
> > currently works even on x86.
> > 
> >>> Do we?  There's no reason a blktap2 kernel module couldn't be built on
> >>> ARM, is there?
> >>
> >> Maybe not, but I am pretty sure that it doesn't work at the moment. I
> >> don't think that the userspace stuff even compiles on ARM.
> >> Eventually we might have blktap on ARM, but I don't want to enable
> >> stuff in Raisin that we know it does not work.
> > 
> > Especially if it is already to a greater or lesser extent deprecated (in
> > favour of eventual blktap3) even on x86.
> 
> So from my discussions w/ the XenServer guys, it seems that:
> 
> 1. The "master" branch of the blktap.git repo contains support for
> *both* blktap3 and blktap2.5 (with a kernel module)
> 
> 2. XenServer uses blktap3 for guest access, but still use the blktap2.5
> w/ kernel module for dom0 access to guest disks, to avoid the
> possibility of hitting a scalability limit due to grant references.
> 
> So from raisin's perspective, the only difference between blktap2.5 and
> blktap3 is using the "master" branch rather than the "blktap2" branch of
> the repo.

That is not entirely true: compiling and installing a kernel module is
quite different from userspace stuff, at least in terms of dependencies
and installation paths.


> Whether we maintain support for blktap2.5 in libxl is a matter for the
> Xen maintainers; but if xapi is ever going to start using libxl, it will
> certainly need to be able to do so.
> 
> (Dave / David, please correct me if I'm wrong.)
> 
> That said, there's no harm in disabling it on ARM to begin with, and
> enabling it once blktap3 works.

Yes, I would the code in Raisin to actually work :-)

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

On 04/21/2015 11:09 AM, Stefano Stabellini wrote:
> On Tue, 21 Apr 2015, George Dunlap wrote:
>> On 04/21/2015 10:25 AM, Ian Campbell wrote:
>>> On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
>>>>>> I think we need to disable the build on architectures other than x86,
>>>>>> see grub for example
>>>
>>> Eventually we might want to build our own grub on ARM in order to pick
>>> up Fu Wei's multiboot for arm64 patches, until they enter distros?
>>>
>>> Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
>>> directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
>>> currently works even on x86.
>>>
>>>>> Do we?  There's no reason a blktap2 kernel module couldn't be built on
>>>>> ARM, is there?
>>>>
>>>> Maybe not, but I am pretty sure that it doesn't work at the moment. I
>>>> don't think that the userspace stuff even compiles on ARM.
>>>> Eventually we might have blktap on ARM, but I don't want to enable
>>>> stuff in Raisin that we know it does not work.
>>>
>>> Especially if it is already to a greater or lesser extent deprecated (in
>>> favour of eventual blktap3) even on x86.
>>
>> So from my discussions w/ the XenServer guys, it seems that:
>>
>> 1. The "master" branch of the blktap.git repo contains support for
>> *both* blktap3 and blktap2.5 (with a kernel module)
>>
>> 2. XenServer uses blktap3 for guest access, but still use the blktap2.5
>> w/ kernel module for dom0 access to guest disks, to avoid the
>> possibility of hitting a scalability limit due to grant references.
>>
>> So from raisin's perspective, the only difference between blktap2.5 and
>> blktap3 is using the "master" branch rather than the "blktap2" branch of
>> the repo.
> 
> That is not entirely true: compiling and installing a kernel module is
> quite different from userspace stuff, at least in terms of dependencies
> and installation paths.

The blktap.git repo doesn't compile a kernel module; it's only building
userspace binaries and libraries.  Libxl already knows how to detect the
presence or absence of the kernel module and behave accordingly.

>> Whether we maintain support for blktap2.5 in libxl is a matter for the
>> Xen maintainers; but if xapi is ever going to start using libxl, it will
>> certainly need to be able to do so.
>>
>> (Dave / David, please correct me if I'm wrong.)
>>
>> That said, there's no harm in disabling it on ARM to begin with, and
>> enabling it once blktap3 works.
> 
> Yes, I would the code in Raisin to actually work :-)

The code should work just fine.  When run on an ARM system without a
blktap2 kernel module, libxl should detect that blktap2 is not available
and not use it.  If someone builds their own ARM kernel with blktap2
enabled, it will use it.

The only reason to disable this on ARM at the minute is because you
*believe* that nobody will ever want to build the blktap2 kernel module
on ARM, and so you want to avoid the build overhead and space overhead
of compiling and using dead code.  If that's your goal, you might get
more mileage out of disabling stuff like xenmon or the paging code. :-)

There's already an ARM Server SIG for CentOS; once that gets completed,
Xen4CentOS project will probably also do an ARM server port, at which
point it will almost certainly attempt to port over the blktap2 kernel
modules.

Enabling blktap.git on ARM by default means a bit of extra compilation
time and code in the resulting binary (though not much at all).
Disabling it on ARM means that we'll have to enable it again once either
1) we get blktap3 working, or 2) someone shows an interest in using
blktap2 on ARM.

Both costs are so low as to make it a bike-shed issue in my mind.  I'd
paint the bike shed "Enable it by arm on default", but repainting the
shed when the time comes will be easy, so I don't care that much.  I
just want to make it clear that it *is* a bike-shed issue. :-)

 -George

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Stefano Stabellini]

On Tue, 21 Apr 2015, George Dunlap wrote:
> On 04/21/2015 11:09 AM, Stefano Stabellini wrote:
> > On Tue, 21 Apr 2015, George Dunlap wrote:
> >> On 04/21/2015 10:25 AM, Ian Campbell wrote:
> >>> On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
> >>>>>> I think we need to disable the build on architectures other than x86,
> >>>>>> see grub for example
> >>>
> >>> Eventually we might want to build our own grub on ARM in order to pick
> >>> up Fu Wei's multiboot for arm64 patches, until they enter distros?
> >>>
> >>> Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
> >>> directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
> >>> currently works even on x86.
> >>>
> >>>>> Do we?  There's no reason a blktap2 kernel module couldn't be built on
> >>>>> ARM, is there?
> >>>>
> >>>> Maybe not, but I am pretty sure that it doesn't work at the moment. I
> >>>> don't think that the userspace stuff even compiles on ARM.
> >>>> Eventually we might have blktap on ARM, but I don't want to enable
> >>>> stuff in Raisin that we know it does not work.
> >>>
> >>> Especially if it is already to a greater or lesser extent deprecated (in
> >>> favour of eventual blktap3) even on x86.
> >>
> >> So from my discussions w/ the XenServer guys, it seems that:
> >>
> >> 1. The "master" branch of the blktap.git repo contains support for
> >> *both* blktap3 and blktap2.5 (with a kernel module)
> >>
> >> 2. XenServer uses blktap3 for guest access, but still use the blktap2.5
> >> w/ kernel module for dom0 access to guest disks, to avoid the
> >> possibility of hitting a scalability limit due to grant references.
> >>
> >> So from raisin's perspective, the only difference between blktap2.5 and
> >> blktap3 is using the "master" branch rather than the "blktap2" branch of
> >> the repo.
> > 
> > That is not entirely true: compiling and installing a kernel module is
> > quite different from userspace stuff, at least in terms of dependencies
> > and installation paths.
> 
> The blktap.git repo doesn't compile a kernel module; it's only building
> userspace binaries and libraries.  Libxl already knows how to detect the
> presence or absence of the kernel module and behave accordingly.
> 
> >> Whether we maintain support for blktap2.5 in libxl is a matter for the
> >> Xen maintainers; but if xapi is ever going to start using libxl, it will
> >> certainly need to be able to do so.
> >>
> >> (Dave / David, please correct me if I'm wrong.)
> >>
> >> That said, there's no harm in disabling it on ARM to begin with, and
> >> enabling it once blktap3 works.
> > 
> > Yes, I would the code in Raisin to actually work :-)
> 
> The code should work just fine.  When run on an ARM system without a
> blktap2 kernel module, libxl should detect that blktap2 is not available
> and not use it.  If someone builds their own ARM kernel with blktap2
> enabled, it will use it.
> 
> The only reason to disable this on ARM at the minute is because you
> *believe* that nobody will ever want to build the blktap2 kernel module
> on ARM, and so you want to avoid the build overhead and space overhead
> of compiling and using dead code.  If that's your goal, you might get
> more mileage out of disabling stuff like xenmon or the paging code. :-)

No, that is not right. The reason I would like to disable blktap2 for
ARM is that the userspace binaries and libraries won't compile on ARM at
the moment. At least that was the case the last time I tried it. If you
prove me wrong, I would be happy to have blktap2 on ARM too.


> There's already an ARM Server SIG for CentOS; once that gets completed,
> Xen4CentOS project will probably also do an ARM server port, at which
> point it will almost certainly attempt to port over the blktap2 kernel
> modules.
> 
> Enabling blktap.git on ARM by default means a bit of extra compilation
> time and code in the resulting binary (though not much at all).
> Disabling it on ARM means that we'll have to enable it again once either
> 1) we get blktap3 working, or 2) someone shows an interest in using
> blktap2 on ARM.
> 
> Both costs are so low as to make it a bike-shed issue in my mind.  I'd
> paint the bike shed "Enable it by arm on default", but repainting the
> shed when the time comes will be easy, so I don't care that much.  I
> just want to make it clear that it *is* a bike-shed issue. :-)

Right, I am not worried about the build time.

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

On 04/21/2015 11:42 AM, Stefano Stabellini wrote:
>> The only reason to disable this on ARM at the minute is because you
>> *believe* that nobody will ever want to build the blktap2 kernel module
>> on ARM, and so you want to avoid the build overhead and space overhead
>> of compiling and using dead code.  If that's your goal, you might get
>> more mileage out of disabling stuff like xenmon or the paging code. :-)
> 
> No, that is not right. The reason I would like to disable blktap2 for
> ARM is that the userspace binaries and libraries won't compile on ARM at
> the moment. At least that was the case the last time I tried it. If you
> prove me wrong, I would be happy to have blktap2 on ARM too.

Right, I didn't realize that.  That's certainly a good reason to disable
it on ARM. :-)

 -George

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Dave Scott]

> On 21 Apr 2015, at 11:42, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com> wrote:
> 
> On Tue, 21 Apr 2015, George Dunlap wrote:
>> On 04/21/2015 11:09 AM, Stefano Stabellini wrote:
>>> On Tue, 21 Apr 2015, George Dunlap wrote:
>>>> On 04/21/2015 10:25 AM, Ian Campbell wrote:
>>>>> On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
>>>>>>>> I think we need to disable the build on architectures other than x86,
>>>>>>>> see grub for example
>>>>> 
>>>>> Eventually we might want to build our own grub on ARM in order to pick
>>>>> up Fu Wei's multiboot for arm64 patches, until they enter distros?
>>>>> 
>>>>> Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
>>>>> directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
>>>>> currently works even on x86.
>>>>> 
>>>>>>> Do we?  There's no reason a blktap2 kernel module couldn't be built on
>>>>>>> ARM, is there?
>>>>>> 
>>>>>> Maybe not, but I am pretty sure that it doesn't work at the moment. I
>>>>>> don't think that the userspace stuff even compiles on ARM.
>>>>>> Eventually we might have blktap on ARM, but I don't want to enable
>>>>>> stuff in Raisin that we know it does not work.
>>>>> 
>>>>> Especially if it is already to a greater or lesser extent deprecated (in
>>>>> favour of eventual blktap3) even on x86.
>>>> 
>>>> So from my discussions w/ the XenServer guys, it seems that:
>>>> 
>>>> 1. The "master" branch of the blktap.git repo contains support for
>>>> *both* blktap3 and blktap2.5 (with a kernel module)
>>>> 
>>>> 2. XenServer uses blktap3 for guest access, but still use the blktap2.5
>>>> w/ kernel module for dom0 access to guest disks, to avoid the
>>>> possibility of hitting a scalability limit due to grant references.
>>>> 
>>>> So from raisin's perspective, the only difference between blktap2.5 and
>>>> blktap3 is using the "master" branch rather than the "blktap2" branch of
>>>> the repo.
>>> 
>>> That is not entirely true: compiling and installing a kernel module is
>>> quite different from userspace stuff, at least in terms of dependencies
>>> and installation paths.
>> 
>> The blktap.git repo doesn't compile a kernel module; it's only building
>> userspace binaries and libraries.  Libxl already knows how to detect the
>> presence or absence of the kernel module and behave accordingly.
>> 
>>>> Whether we maintain support for blktap2.5 in libxl is a matter for the
>>>> Xen maintainers; but if xapi is ever going to start using libxl, it will
>>>> certainly need to be able to do so.
>>>> 
>>>> (Dave / David, please correct me if I'm wrong.)
>>>> 
>>>> That said, there's no harm in disabling it on ARM to begin with, and
>>>> enabling it once blktap3 works.
>>> 
>>> Yes, I would the code in Raisin to actually work :-)
>> 
>> The code should work just fine.  When run on an ARM system without a
>> blktap2 kernel module, libxl should detect that blktap2 is not available
>> and not use it.  If someone builds their own ARM kernel with blktap2
>> enabled, it will use it.
>> 
>> The only reason to disable this on ARM at the minute is because you
>> *believe* that nobody will ever want to build the blktap2 kernel module
>> on ARM, and so you want to avoid the build overhead and space overhead
>> of compiling and using dead code.  If that's your goal, you might get
>> more mileage out of disabling stuff like xenmon or the paging code. :-)
> 
> No, that is not right. The reason I would like to disable blktap2 for
> ARM is that the userspace binaries and libraries won't compile on ARM at
> the moment. At least that was the case the last time I tried it. If you
> prove me wrong, I would be happy to have blktap2 on ARM too.

FYI I’ve been sporadically building the blktap userspace on ARM and it seems to work, although last time I had to fix some format strings which assumed a 64-bit architecture. I think that patch got merged. I think my last build was in January.

The Mirage xen-arm-builder images currently contain a patched kernel with the blktap driver added as a patch, and a build of xapi + libxl + the blktap userspace tools. It’s only been lightly tested though :-)

Cheers,
Dave

> 
> 
>> There's already an ARM Server SIG for CentOS; once that gets completed,
>> Xen4CentOS project will probably also do an ARM server port, at which
>> point it will almost certainly attempt to port over the blktap2 kernel
>> modules.
>> 
>> Enabling blktap.git on ARM by default means a bit of extra compilation
>> time and code in the resulting binary (though not much at all).
>> Disabling it on ARM means that we'll have to enable it again once either
>> 1) we get blktap3 working, or 2) someone shows an interest in using
>> blktap2 on ARM.
>> 
>> Both costs are so low as to make it a bike-shed issue in my mind.  I'd
>> paint the bike shed "Enable it by arm on default", but repainting the
>> shed when the time comes will be easy, so I don't care that much.  I
>> just want to make it clear that it *is* a bike-shed issue. :-)
> 
> Right, I am not worried about the build time.

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

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Dave Scott]

> On 21 Apr 2015, at 12:01, Dave Scott <Dave.Scott@citrix.com> wrote:
> 
>> 
>> On 21 Apr 2015, at 11:42, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com> wrote:
>> 
>> On Tue, 21 Apr 2015, George Dunlap wrote:
>>> On 04/21/2015 11:09 AM, Stefano Stabellini wrote:
>>>> On Tue, 21 Apr 2015, George Dunlap wrote:
>>>>> On 04/21/2015 10:25 AM, Ian Campbell wrote:
>>>>>> On Mon, 2015-04-20 at 18:05 +0100, Stefano Stabellini wrote:
>>>>>>>>> I think we need to disable the build on architectures other than x86,
>>>>>>>>> see grub for example
>>>>>> 
>>>>>> Eventually we might want to build our own grub on ARM in order to pick
>>>>>> up Fu Wei's multiboot for arm64 patches, until they enter distros?
>>>>>> 
>>>>>> Or maybe Raisin on UEFI should be calling efibootmgr to register Xen
>>>>>> directly with the BIOS, and creating a xen.cfg in /boot, i.e. the way it
>>>>>> currently works even on x86.
>>>>>> 
>>>>>>>> Do we?  There's no reason a blktap2 kernel module couldn't be built on
>>>>>>>> ARM, is there?
>>>>>>> 
>>>>>>> Maybe not, but I am pretty sure that it doesn't work at the moment. I
>>>>>>> don't think that the userspace stuff even compiles on ARM.
>>>>>>> Eventually we might have blktap on ARM, but I don't want to enable
>>>>>>> stuff in Raisin that we know it does not work.
>>>>>> 
>>>>>> Especially if it is already to a greater or lesser extent deprecated (in
>>>>>> favour of eventual blktap3) even on x86.
>>>>> 
>>>>> So from my discussions w/ the XenServer guys, it seems that:
>>>>> 
>>>>> 1. The "master" branch of the blktap.git repo contains support for
>>>>> *both* blktap3 and blktap2.5 (with a kernel module)
>>>>> 
>>>>> 2. XenServer uses blktap3 for guest access, but still use the blktap2.5
>>>>> w/ kernel module for dom0 access to guest disks, to avoid the
>>>>> possibility of hitting a scalability limit due to grant references.
>>>>> 
>>>>> So from raisin's perspective, the only difference between blktap2.5 and
>>>>> blktap3 is using the "master" branch rather than the "blktap2" branch of
>>>>> the repo.
>>>> 
>>>> That is not entirely true: compiling and installing a kernel module is
>>>> quite different from userspace stuff, at least in terms of dependencies
>>>> and installation paths.
>>> 
>>> The blktap.git repo doesn't compile a kernel module; it's only building
>>> userspace binaries and libraries.  Libxl already knows how to detect the
>>> presence or absence of the kernel module and behave accordingly.
>>> 
>>>>> Whether we maintain support for blktap2.5 in libxl is a matter for the
>>>>> Xen maintainers; but if xapi is ever going to start using libxl, it will
>>>>> certainly need to be able to do so.
>>>>> 
>>>>> (Dave / David, please correct me if I'm wrong.)
>>>>> 
>>>>> That said, there's no harm in disabling it on ARM to begin with, and
>>>>> enabling it once blktap3 works.
>>>> 
>>>> Yes, I would the code in Raisin to actually work :-)
>>> 
>>> The code should work just fine.  When run on an ARM system without a
>>> blktap2 kernel module, libxl should detect that blktap2 is not available
>>> and not use it.  If someone builds their own ARM kernel with blktap2
>>> enabled, it will use it.
>>> 
>>> The only reason to disable this on ARM at the minute is because you
>>> *believe* that nobody will ever want to build the blktap2 kernel module
>>> on ARM, and so you want to avoid the build overhead and space overhead
>>> of compiling and using dead code.  If that's your goal, you might get
>>> more mileage out of disabling stuff like xenmon or the paging code. :-)
>> 
>> No, that is not right. The reason I would like to disable blktap2 for
>> ARM is that the userspace binaries and libraries won't compile on ARM at
>> the moment. At least that was the case the last time I tried it. If you
>> prove me wrong, I would be happy to have blktap2 on ARM too.
> 
> FYI I’ve been sporadically building the blktap userspace on ARM and it seems to work, although last time I had to fix some format strings which assumed a 64-bit architecture. I think that patch got merged. I think my last build was in January.
> 
> The Mirage xen-arm-builder images currently contain a patched kernel with the blktap driver added as a patch, and a build of xapi + libxl + the blktap userspace tools. It’s only been lightly tested though :-)

Quick followup, I’ve been building the tools with this patch:

https://github.com/xenserver/buildroot/blob/master/SOURCES/blktap-gntcpy.patch

This allows the build without the grant copy support.

> 
> Cheers,
> Dave
> 
>> 
>> 
>>> There's already an ARM Server SIG for CentOS; once that gets completed,
>>> Xen4CentOS project will probably also do an ARM server port, at which
>>> point it will almost certainly attempt to port over the blktap2 kernel
>>> modules.
>>> 
>>> Enabling blktap.git on ARM by default means a bit of extra compilation
>>> time and code in the resulting binary (though not much at all).
>>> Disabling it on ARM means that we'll have to enable it again once either
>>> 1) we get blktap3 working, or 2) someone shows an interest in using
>>> blktap2 on ARM.
>>> 
>>> Both costs are so low as to make it a bike-shed issue in my mind.  I'd
>>> paint the bike shed "Enable it by arm on default", but repainting the
>>> shed when the time comes will be easy, so I don't care that much.  I
>>> just want to make it clear that it *is* a bike-shed issue. :-)
>> 
>> Right, I am not worried about the build time.

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

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[George Dunlap]

On 04/21/2015 12:04 PM, Dave Scott wrote:
>> FYI I’ve been sporadically building the blktap userspace on ARM and it seems to work, although last time I had to fix some format strings which assumed a 64-bit architecture. I think that patch got merged. I think my last build was in January.
>>
>> The Mirage xen-arm-builder images currently contain a patched kernel with the blktap driver added as a patch, and a build of xapi + libxl + the blktap userspace tools. It’s only been lightly tested though :-)
> 
> Quick followup, I’ve been building the tools with this patch:
> 
> https://github.com/xenserver/buildroot/blob/master/SOURCES/blktap-gntcpy.patch
> 
> This allows the build without the grant copy support.

This is the master branch, then?

 -George


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

Re: [PATCH 8/8] raisin: RFC Add blktap2 as an external tree

[Dave Scott]

> On 21 Apr 2015, at 12:06, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> 
> On 04/21/2015 12:04 PM, Dave Scott wrote:
>>> FYI I’ve been sporadically building the blktap userspace on ARM and it seems to work, although last time I had to fix some format strings which assumed a 64-bit architecture. I think that patch got merged. I think my last build was in January.
>>> 
>>> The Mirage xen-arm-builder images currently contain a patched kernel with the blktap driver added as a patch, and a build of xapi + libxl + the blktap userspace tools. It’s only been lightly tested though :-)
>> 
>> Quick followup, I’ve been building the tools with this patch:
>> 
>> https://github.com/xenserver/buildroot/blob/master/SOURCES/blktap-gntcpy.patch
>> 
>> This allows the build without the grant copy support.
> 
> This is the master branch, then?

I’m using an rpm .spec file:

https://github.com/xenserver/buildroot/blob/master/SPECS/blktap.spec

which points to a tag. It’s currently v3.0.50 which looks like it’s the latest commit on the master branch:

https://github.com/xapi-project/blktap/commit/ee27d3f812ecc545d286b946d247eb782833b405

However I’ve not built this exact version on ARM yet. My old ARM binaries (from Feb 2015 rather than Jan) are tagged 0.9.3.fe874d-1 which appears to correspond to this version of the spec file:

https://github.com/xenserver/buildroot/blob/7fe65d137b6d759c80fb8f6d150f8df884ca9117/SPECS/blktap.spec

Cheers,
Dave

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