[PATCH] Fix error: array subscript has type 'char'

[PATCH] Fix error: array subscript has type 'char'

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Use unsigned char variable, or cast to (unsigned char), for
tolower()/islower() and friends. Fix compiler error
array subscript has type 'char' [-Werror=char-subscripts]

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/light/libxl_qmp.c | 2 +-
 tools/xentrace/xentrace.c    | 2 +-
 xen/tools/symbols.c          | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libs/light/libxl_qmp.c b/tools/libs/light/libxl_qmp.c
index c394000ea9..9b638e6f54 100644
--- a/tools/libs/light/libxl_qmp.c
+++ b/tools/libs/light/libxl_qmp.c
@@ -1249,7 +1249,7 @@ static int qmp_error_class_to_libxl_error_code(libxl__gc *gc,
                 se++;
                 continue;
             }
-            if (tolower(*s) != tolower(*se))
+            if (tolower((unsigned char)*s) != tolower((unsigned char)*se))
                 break;
             s++, se++;
         }
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 4b50b8a53e..a8903ebf46 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -957,7 +957,7 @@ static int parse_cpumask_range(const char *mask_str, xc_cpumap_t map)
 {
     unsigned int a, b;
     int nmaskbits;
-    char c;
+    unsigned char c;
     int in_range;
     const char *s;
 
diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
index 9f9e2c9900..0b12452616 100644
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -173,11 +173,11 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */
 	s->len = strlen(str) + 1;
-	if (islower(stype) && filename)
+	if (islower((unsigned char)stype) && filename)
 		s->len += strlen(filename) + 1;
 	s->sym = malloc(s->len + 1);
 	sym = SYMBOL_NAME(s);
-	if (islower(stype) && filename) {
+	if (islower((unsigned char)stype) && filename) {
 		sym = stpcpy(sym, filename);
 		*sym++ = '#';
 	}
-- 
2.29.2

[PATCH] NetBSD: Fix lock directory path

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD the lock directory is in /var/run/

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 m4/paths.m4     | 2 +-
 tools/configure | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/m4/paths.m4 b/m4/paths.m4
index 89d3bb8312..1c107b1a61 100644
--- a/m4/paths.m4
+++ b/m4/paths.m4
@@ -142,7 +142,7 @@ AC_SUBST(XEN_SCRIPT_DIR)
 
 case "$host_os" in
 *freebsd*) XEN_LOCK_DIR=$localstatedir/lib ;;
-*netbsd*) XEN_LOCK_DIR=$localstatedir/lib ;;
+*netbsd*) XEN_LOCK_DIR=$rundir_path ;;
 *) XEN_LOCK_DIR=$localstatedir/lock ;;
 esac
 AC_SUBST(XEN_LOCK_DIR)
diff --git a/tools/configure b/tools/configure
index 8a708e9baa..131112c41e 100755
--- a/tools/configure
+++ b/tools/configure
@@ -4030,7 +4030,7 @@ XEN_SCRIPT_DIR=$XEN_CONFIG_DIR/scripts
 
 case "$host_os" in
 *freebsd*) XEN_LOCK_DIR=$localstatedir/lib ;;
-*netbsd*) XEN_LOCK_DIR=$localstatedir/lib ;;
+*netbsd*) XEN_LOCK_DIR=$localstatedir/run ;;
 *) XEN_LOCK_DIR=$localstatedir/lock ;;
 esac
 
-- 
2.29.2

Re: [PATCH] NetBSD: Fix lock directory path

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:22PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> On NetBSD the lock directory is in /var/run/
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] NetBSD: Fix lock directory path

[Manuel Bouyer]

On Fri, Jan 15, 2021 at 04:09:19PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:22PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > On NetBSD the lock directory is in /var/run/
> > 
> > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

thanks
I already asked, but ...
should I resend the patch with this tag, or will the commiter add it itself ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] NetBSD: Fix lock directory path

[Andrew Cooper]

On 15/01/2021 15:13, Manuel Bouyer wrote:
> On Fri, Jan 15, 2021 at 04:09:19PM +0100, Roger Pau Monné wrote:
>> On Tue, Jan 12, 2021 at 07:12:22PM +0100, Manuel Bouyer wrote:
>>> From: Manuel Bouyer <bouyer@netbsd.org>
>>>
>>> On NetBSD the lock directory is in /var/run/
>>>
>>> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> thanks
> I already asked, but ...
> should I resend the patch with this tag, or will the commiter add it itself ?

Please accumulate tags yourself, so that if you need to send out another
version of the series, they're already included.

However, this patch is trivial, so I'll commit it right now.  The same
might be true in practice of other patches as well.

~Andrew

[PATCH] NetBSD hotplug: Introduce locking functions

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD, some block device configuration requires serialisation.
Introcuce locking functions, and use them in the block script where
appropriate.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/hotplug/NetBSD/Makefile   |  1 +
 tools/hotplug/NetBSD/block      |  5 ++-
 tools/hotplug/NetBSD/locking.sh | 72 +++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 tools/hotplug/NetBSD/locking.sh

diff --git a/tools/hotplug/NetBSD/Makefile b/tools/hotplug/NetBSD/Makefile
index 6926885ab8..114b223207 100644
--- a/tools/hotplug/NetBSD/Makefile
+++ b/tools/hotplug/NetBSD/Makefile
@@ -3,6 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 # Xen script dir and scripts to go there.
 XEN_SCRIPTS =
+XEN_SCRIPTS += locking.sh
 XEN_SCRIPTS += block
 XEN_SCRIPTS += vif-bridge
 XEN_SCRIPTS += vif-ip
diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
index 32c20b6c89..23c8e38ebf 100644
--- a/tools/hotplug/NetBSD/block
+++ b/tools/hotplug/NetBSD/block
@@ -6,6 +6,7 @@
 
 DIR=$(dirname "$0")
 . "${DIR}/hotplugpath.sh"
+. "${DIR}/locking.sh"
 
 PATH=${bindir}:${sbindir}:${LIBEXEC_BIN}:/bin:/usr/bin:/sbin:/usr/sbin
 export PATH
@@ -62,6 +63,7 @@ case $xstatus in
 			available_disks="$available_disks $disk"
 			eval $disk=free
 		done
+		claim_lock block
 		# Mark the used vnd(4) devices as ``used''.
 		for disk in `sysctl hw.disknames`; do
 			case $disk in
@@ -77,6 +79,7 @@ case $xstatus in
 				break	
 			fi
 		done
+		release_lock block
 		if [ x$device = x ] ; then
 			error "no available vnd device"
 		fi
@@ -86,7 +89,7 @@ case $xstatus in
 		device=$xparams
 		;;
 	esac
-	physical_device=$(stat -f '%r' "$device")
+	physical_device=$(stat -L -f '%r' "$device")
 	xenstore-write $xpath/physical-device $physical_device
 	xenstore-write $xpath/hotplug-status connected
 	exit 0
diff --git a/tools/hotplug/NetBSD/locking.sh b/tools/hotplug/NetBSD/locking.sh
new file mode 100644
index 0000000000..88257f62b7
--- /dev/null
+++ b/tools/hotplug/NetBSD/locking.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+#
+# Copyright (c) 2016, Christoph Badura.  All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+# 1. Redistributions of source code must retain the above copyright
+#    notice, this list of conditions and the following disclaimer.
+# 2. Redistributions in binary form must reproduce the above copyright
+#    notice, this list of conditions and the following disclaimer in the
+#    documentation and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS
+# OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT,
+# INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+#
+
+LOCK_BASEDIR="$XEN_LOCK_DIR/xen-hotplug"
+
+_lockfd=9
+_have_lock=0	# lock not taken yet.
+
+SHLOCK="shlock ${_shlock_debug-}"
+
+_lock_set_vars() {
+	_lockfile="$LOCK_BASEDIR/$1.lock"
+	_lockfifo="$LOCK_BASEDIR/$1.fifo"
+}
+
+_lock_init() {
+	mkdir -p "$LOCK_BASEDIR" 2>/dev/null || true
+	mkfifo $_lockfifo 2>/dev/null || true
+}
+
+#
+# use a named pipe as condition variable
+# opening for read-only blocks when there's no writer.
+# opening for read-write never blocks but unblocks any waiting readers.
+# 
+_lock_wait_cv() {
+	eval "exec $_lockfd<  $_lockfifo ; exec $_lockfd<&-"
+}
+_lock_signal_cv() {
+	eval "exec $_lockfd<> $_lockfifo ; exec $_lockfd<&-"
+}
+
+claim_lock() {
+	_lock_set_vars $1
+	_lock_init
+	until $SHLOCK -f $_lockfile -p $$; do
+		_lock_wait_cv
+	done
+	_have_lock=1
+	# be sure to release the lock when the shell exits
+	trap "release_lock $1" 0 1 2 15
+}
+
+release_lock() {
+	_lock_set_vars $1
+	[ "$_have_lock" != 0 -a -f $_lockfile ] && rm $_lockfile
+	_have_lock=0
+	_lock_signal_cv;
+}
-- 
2.29.2

Re: [PATCH] NetBSD hotplug: Introduce locking functions

[Ian Jackson]

Manuel Bouyer writes ("[PATCH] NetBSD hotplug: Introduce locking functions"):
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> On NetBSD, some block device configuration requires serialisation.
> Introcuce locking functions, and use them in the block script where
> appropriate.

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH] NetBSD hotplug: Introduce locking functions

[Manuel Bouyer]

On Wed, Jan 27, 2021 at 03:57:06PM +0000, Ian Jackson wrote:
> Manuel Bouyer writes ("[PATCH] NetBSD hotplug: Introduce locking functions"):
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > On NetBSD, some block device configuration requires serialisation.
> > Introcuce locking functions, and use them in the block script where
> > appropriate.
> 
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

thanks, but I submitted a v2 patch which uses a locking.sh derived
from the linux one, based on your feedback.
Should I add your Reviewed-by to the v2 ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] NetBSD hotplug: Introduce locking functions

[Ian Jackson]

Manuel Bouyer writes ("Re: [PATCH] NetBSD hotplug: Introduce locking functions"):
> thanks, but I submitted a v2 patch which uses a locking.sh derived
> from the linux one, based on your feedback.
> Should I add your Reviewed-by to the v2 ?

Sorry, yes, please.

Ian.

[PATCH] NetBSD hotplug: fix block unconfigure on destroy

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

When a domain is destroyed, xparams may not be available any more when
the block script is called to unconfigure the vnd.
Check xparam only at configure time, and just unconfigure any vnd present
in the xenstore.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/hotplug/NetBSD/block | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
index 23c8e38ebf..27f3b98335 100644
--- a/tools/hotplug/NetBSD/block
+++ b/tools/hotplug/NetBSD/block
@@ -22,37 +22,28 @@ error() {
 xpath=$1
 xstatus=$2
 xparams=$(xenstore-read "$xpath/params")
-if [ -b "$xparams" ]; then
-	xtype="phy"
-elif [ -f "$xparams" ]; then
-	xtype="file"
-elif [ -z "$xparams" ]; then
-	error "$xpath/params is empty, unable to attach block device."
-else
-	error "$xparams is not a valid file type to use as block device." \
-	      "Only block and regular image files accepted."
-fi
 
 case $xstatus in
 6)
 	# device removed
-	case $xtype in
-	file)
-		vnd=$(xenstore-read "$xpath/vnd" || echo none)
-		if [ $vnd != none ]; then
-			vnconfig -u $vnd
-		fi
-		;;
-	phy)
-		;;
-	*)
-		echo "unknown type $xtype" >&2
-		;;
-	esac
+	vnd=$(xenstore-read "$xpath/vnd" || echo none)
+	if [ $vnd != none ]; then
+		vnconfig -u $vnd
+	fi
 	xenstore-rm $xpath
 	exit 0
 	;;
 2)
+	if [ -b "$xparams" ]; then
+		xtype="phy"
+	elif [ -f "$xparams" ]; then
+		xtype="file"
+	elif [ -z "$xparams" ]; then
+		error "$xpath/params is empty, unable to attach block device."
+	else
+		error "$xparams is not a valid file type to use as block device." \
+		      "Only block and regular image files accepted."
+	fi
 	case $xtype in
 	file)
 		# Store the list of available vnd(4) devices in
-- 
2.29.2

Re: [PATCH] NetBSD hotplug: fix block unconfigure on destroy

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:24PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> When a domain is destroyed, xparams may not be available any more when
> the block script is called to unconfigure the vnd.
> Check xparam only at configure time, and just unconfigure any vnd present
> in the xenstore.

The patch itself seems fine to me, there's no need to fetch params
for unplug, you can just reply on the vnd node.

I'm however not sure why params would be deleted from xenstore but not
vnd, do you know why?

> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

For the code itself:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] NetBSD hotplug: fix block unconfigure on destroy

[Manuel Bouyer]

On Fri, Jan 15, 2021 at 04:27:12PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:24PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > When a domain is destroyed, xparams may not be available any more when
> > the block script is called to unconfigure the vnd.
> > Check xparam only at configure time, and just unconfigure any vnd present
> > in the xenstore.
> 
> The patch itself seems fine to me, there's no need to fetch params
> for unplug, you can just reply on the vnd node.
> 
> I'm however not sure why params would be deleted from xenstore but not
> vnd, do you know why?

I'm not sure, it happens on xl destroy, when the kernel in the
domU is stuck (so it won't shutdown properly).

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] NetBSD hotplug: fix block unconfigure on destroy

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 05:47:49PM +0100, Manuel Bouyer wrote:
> On Fri, Jan 15, 2021 at 04:27:12PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 12, 2021 at 07:12:24PM +0100, Manuel Bouyer wrote:
> > > From: Manuel Bouyer <bouyer@netbsd.org>
> > > 
> > > When a domain is destroyed, xparams may not be available any more when
> > > the block script is called to unconfigure the vnd.
> > > Check xparam only at configure time, and just unconfigure any vnd present
> > > in the xenstore.
> > 
> > The patch itself seems fine to me, there's no need to fetch params
> > for unplug, you can just reply on the vnd node.
> > 
> > I'm however not sure why params would be deleted from xenstore but not
> > vnd, do you know why?
> 
> I'm not sure, it happens on xl destroy, when the kernel in the
> domU is stuck (so it won't shutdown properly).

That's weird. Linux hotplug script will unconditionally read the
params node and we had no complaints there. Can you assert this still
happens with the latest version of Xen?

As said I think fetching vnd on detach is fine because there's no need
to fetch the other nodes, but this seems to be masking some kind of
error elsewhere.

Thanks, Roger.

Re: [PATCH] NetBSD hotplug: fix block unconfigure on destroy

[Manuel Bouyer]

On Wed, Jan 27, 2021 at 10:40:43AM +0100, Roger Pau Monné wrote:
> That's weird. Linux hotplug script will unconditionally read the
> params node and we had no complaints there. Can you assert this still
> happens with the latest version of Xen?

It does with Xen 4.13 for sure. 

> 
> As said I think fetching vnd on detach is fine because there's no need
> to fetch the other nodes, but this seems to be masking some kind of
> error elsewhere.

My priority is to get the patches in at this point. Each round takes me
hours to get them in shape.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] NetBSD hotplug: handle case where vifname is not present

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Some Xen version didn't set the vifname in xenstore; just build one if
not present.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/hotplug/NetBSD/vif-bridge | 5 ++++-
 tools/hotplug/NetBSD/vif-ip     | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/hotplug/NetBSD/vif-bridge b/tools/hotplug/NetBSD/vif-bridge
index b58e922601..cd428b5936 100644
--- a/tools/hotplug/NetBSD/vif-bridge
+++ b/tools/hotplug/NetBSD/vif-bridge
@@ -23,7 +23,10 @@ case $xstatus in
 	xbridge=$(xenstore-read "$xpath/bridge")
 	xfid=$(xenstore-read "$xpath/frontend-id")
 	xhandle=$(xenstore-read "$xpath/handle")
-	iface=$(xenstore-read "$xpath/vifname")
+	iface=$(xenstore-read "$xpath/vifname") || true
+	if [ x${iface} = "x" ] ; then
+		iface=xvif$xfid.$xhandle
+	fi
 	ifconfig $iface up
 	brconfig $xbridge add $iface
 	xenstore-write $xpath/hotplug-status connected
diff --git a/tools/hotplug/NetBSD/vif-ip b/tools/hotplug/NetBSD/vif-ip
index 83cbfe20e2..944f50f881 100644
--- a/tools/hotplug/NetBSD/vif-ip
+++ b/tools/hotplug/NetBSD/vif-ip
@@ -24,6 +24,10 @@ case $xstatus in
 	xfid=$(xenstore-read "$xpath/frontend-id")
 	xhandle=$(xenstore-read "$xpath/handle")
 	iface=$(xenstore-read "$xpath/vifname")
+	iface=$(xenstore-read "$xpath/vifname") || true
+	if [ x${iface} = "x" ] ; then
+		iface=xvif$xfid.$xhandle
+	fi
 	ifconfig $iface $xip up
 	xenstore-write $xpath/hotplug-status connected
 	exit 0
-- 
2.29.2

Re: [PATCH] NetBSD hotplug: handle case where vifname is not present

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:25PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Some Xen version didn't set the vifname in xenstore; just build one if
> not present.

I think the current version (what's going to become 4.15) should write
the vifname in all cases? If not that's likely an error that we should
fix elsewhere IMO.

Can you check whether the current version has this error present? And
whether it affects PV or HVM guests?

Thanks, Roger.

Re: [PATCH] NetBSD hotplug: handle case where vifname is not present

[Manuel Bouyer]

On Fri, Jan 15, 2021 at 05:06:59PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:25PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > Some Xen version didn't set the vifname in xenstore; just build one if
> > not present.
> 
> I think the current version (what's going to become 4.15) should write
> the vifname in all cases? If not that's likely an error that we should
> fix elsewhere IMO.
> 
> Can you check whether the current version has this error present? And
> whether it affects PV or HVM guests?

Yes, in recent Xen version this doesn't seem to be needed any more.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] NetBSD: remove xenbackendd

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

NetBSD doens't need xenbackendd with xl toolstack so don't build it.
Remove now unused xenbackendd directory/files.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/Makefile                  |   1 -
 tools/xenbackendd/Makefile      |  45 -----
 tools/xenbackendd/xenbackendd.c | 326 --------------------------------
 3 files changed, 372 deletions(-)
 delete mode 100644 tools/xenbackendd/Makefile
 delete mode 100644 tools/xenbackendd/xenbackendd.c

diff --git a/tools/Makefile b/tools/Makefile
index ed71474421..757a560be0 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -18,7 +18,6 @@ SUBDIRS-$(CONFIG_X86) += firmware
 SUBDIRS-y += console
 SUBDIRS-y += xenmon
 SUBDIRS-y += xentop
-SUBDIRS-$(CONFIG_NetBSD) += xenbackendd
 SUBDIRS-y += libfsimage
 SUBDIRS-$(CONFIG_Linux) += vchan
 
diff --git a/tools/xenbackendd/Makefile b/tools/xenbackendd/Makefile
deleted file mode 100644
index ba53bbf7e6..0000000000
--- a/tools/xenbackendd/Makefile
+++ /dev/null
@@ -1,45 +0,0 @@
-# Copyright (c) 2009 Advanced Micro Devices, Inc.
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; under version 2 of the License.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-
-XEN_ROOT=$(CURDIR)/../..
-include $(XEN_ROOT)/tools/Rules.mk
-
-CFLAGS  += -Werror
-CFLAGS  += $(CFLAGS_libxenstore)
-CPPFLAGS += -DXEN_SCRIPT_DIR="\"$(XEN_SCRIPT_DIR)\""
-LDLIBS  += $(LDLIBS_libxenstore)
-
-.PHONY: all
-all: build
-
-.PHONY: build
-build: xenbackendd
-
-.PHONY: install
-install: build
-	$(INSTALL_DIR) $(DESTDIR)$(sbindir)
-	$(INSTALL_PROG) xenbackendd $(DESTDIR)$(sbindir)
-
-.PHONY: clean
-clean:
-	$(RM) *.a *.so *.o $(DEPS_RM) xenbackendd _paths.h
-
-.PHONY: distclean
-distclean: clean
-
-xenbackendd.o: _paths.h
-xenbackendd: xenbackendd.o
-	$(CC) $(LDFLAGS) $< -o $@ $(LDLIBS) $(APPEND_LDFLAGS)
-
-genpath-target = $(call buildmakevars2header,_paths.h)
-$(eval $(genpath-target))
-
--include $(DEPS_INCLUDE)
diff --git a/tools/xenbackendd/xenbackendd.c b/tools/xenbackendd/xenbackendd.c
deleted file mode 100644
index 21884af772..0000000000
--- a/tools/xenbackendd/xenbackendd.c
+++ /dev/null
@@ -1,326 +0,0 @@
-/* $NetBSD: xenbackendd.c,v 1.1.1.1 2008/08/07 20:26:57 cegger Exp $ */
-/*
- * Copyright (C) 2006 Manuel Bouyer <bouyer@netbsd.org>
- * Copyright (C) 2009 Christoph Egger <Christoph.Egger@amd.com>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; under version 2 of the License.
- * 
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- * 
- *  You should have received a copy of the GNU General Public License
- *  along with this program; If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <sys/wait.h>
-#include <unistd.h>
-#include <fcntl.h>
-#include <stdlib.h>
-#include <stdio.h>
-#include <stdarg.h>
-#include <string.h>
-#include <syslog.h>
-
-#include <xenstore.h>
-
-#include "_paths.h"
-
-#define DEVTYPE_UNKNOWN 0
-#define DEVTYPE_VIF 1
-#define DEVTYPE_VBD 2
-#define DISABLE_EXEC "libxl/disable_udev"
-
-#define DOMAIN_PATH "/local/domain/0"
-
-#ifndef XEN_SCRIPT_DIR
-#error XEN_SCRIPT_DIR not defined
-#endif
-
-#ifndef VBD_SCRIPT
-#define VBD_SCRIPT XEN_SCRIPT_DIR"/block"
-#endif
-#ifndef LOG_FILE
-#define LOG_FILE XEN_LOG_DIR "xenbackendd.log"
-#endif
-#ifndef PID_FILE
-#define PID_FILE XEN_RUN_DIR "xenbackendd.pid"
-#endif
-
-
-struct xs_handle *xs;
-
-int fflag = 0;
-int dflag = 0;
-
-const char *vbd_script = NULL;
-const char *log_file = NULL;
-const char *pidfile = NULL;
-
-static void
-dolog(int pri, const char *fmt, ...)
-{
-	va_list ap;
-	va_start(ap, fmt);
-	vfprintf(stderr, fmt, ap);
-	va_end(ap);
-	fprintf(stderr, "\n");
-	fflush(stderr);
-	va_start(ap, fmt);
-	vsyslog(pri, fmt, ap);
-	va_end(ap);
-}
-
-static void
-dodebug(const char *fmt, ...)
-{
-	va_list ap;
-
-	if (dflag == 0)
-		return;
-	va_start(ap, fmt);
-	vfprintf(stdout, fmt, ap);
-	va_end(ap);
-	printf("\n");
-	fflush(stdout);
-}
-
-static void
-doexec(const char *cmd, const char *arg1, const char *arg2)
-{
-	dodebug("exec %s %s %s", cmd, arg1, arg2);
-	switch(vfork()) {
-	case -1:
-		dolog(LOG_ERR, "can't vfork: %s", strerror(errno));
-		break;
-	case 0:
-		execl(cmd, cmd, arg1, arg2, NULL);
-		dolog(LOG_ERR, "can't exec %s: %s", cmd, strerror(errno));
-		exit(EXIT_FAILURE);
-		/* NOTREACHED */
-		break;
-	default:
-		wait(NULL);
-		break;
-	}
-}
-
-static void
-usage(void)
-{
-	fprintf(stderr,
-	    "usage: %s [-d] [-f] [-l log_file] [-p pif_file] [-s vbd_script]\n",
-	    getprogname());
-	exit(EXIT_FAILURE);
-}
-
-static int
-xen_setup(void)
-{
-	xs = xs_open(0);
-	if (xs == NULL) {
-		dolog(LOG_ERR,
-		    "Failed to contact xenstore (%s).  Is it running?",
-		    strerror(errno));
-		goto out;
-	}
-
-	if (!xs_watch(xs, DOMAIN_PATH, "backend")) {
-		dolog(LOG_ERR, "xenstore watch on backend fails.");
-		goto out;
-	}
-	return 0;
-
- out:
-	if (xs) {
-		xs_close(xs);
-		xs = NULL;
-	}
-	return -1;
-}
-
-int
-main(int argc, char * const argv[])
-{
-	char **vec;
-	unsigned int num;
-	char *s;
-	int state;
-	char *sstate, *sdisable;
-	char *p;
-	char buf[80];
-	int type;
-	int ch;
-	int debug_fd;
-	FILE *pidfile_f;
-
-	while ((ch = getopt(argc, argv, "dfl:p:s:")) != -1) {
-		switch (ch) {
-		case 'd':
-			dflag = 1;
-			break;
-		case 'f':
-			fflag = 1;
-			break;
-		case 'l':
-			log_file = optarg;
-			break;
-		case 'p':
-			pidfile = optarg;
-		case 's':
-			vbd_script = optarg;
-			break;
-		default:
-			usage();
-		}
-	}
-
-	if (vbd_script == NULL)
-		vbd_script = VBD_SCRIPT;
-	if (pidfile == NULL)
-		pidfile = PID_FILE;
-	if (log_file == NULL)
-		log_file = LOG_FILE;
-
-	openlog("xenbackendd", LOG_PID | LOG_NDELAY, LOG_DAEMON);
-
-	if (fflag == 0) {
-		/* open log file */
-		debug_fd = open(log_file, O_RDWR | O_CREAT | O_TRUNC, 0644);
-		if (debug_fd == -1) {
-			dolog(LOG_ERR, "can't open %s: %s",
-			    log_file, strerror(errno));
-			exit(EXIT_FAILURE);
-		}
-	}
-
-	if (fflag == 0) {
-		/* daemonize */
-		pidfile_f = fopen(pidfile, "w");
-		if (pidfile_f == NULL) {
-			dolog(LOG_ERR, "can't open %s: %s",
-			    pidfile, strerror(errno));
-			exit(EXIT_FAILURE);
-		}
-		if (daemon(0, 0) < 0) {
-			dolog(LOG_ERR, "can't daemonize: %s",
-			    strerror(errno));
-			exit(EXIT_FAILURE);
-		}
-		fprintf(pidfile_f, "%d\n", (int)getpid());
-		fclose(pidfile_f);
-
-		/* redirect stderr to log file */
-		if (dup2(debug_fd, STDERR_FILENO) < 0) {
-			dolog(LOG_ERR, "can't redirect stderr to %s: %s\n",
-			    log_file, strerror(errno));
-			exit(EXIT_FAILURE);
-		}
-
-		/* also redirect stdout if we're in debug mode */
-		if (dflag) {
-			if (dup2(debug_fd, STDOUT_FILENO) < 0) {
-				dolog(LOG_ERR,
-				    "can't redirect stdout to %s: %s\n",
-				    log_file, strerror(errno));
-				exit(EXIT_FAILURE);
-			}
-		}
-
-		close(debug_fd);
-		debug_fd = -1;
-	}
-
-	if (xen_setup() < 0)
-		exit(EXIT_FAILURE);
-
-	for (;;) {
-		vec = xs_read_watch(xs, &num);
-		dodebug("read from xen watch: %s", *vec);
-		if (!vec) {
-			dolog(LOG_ERR, "xs_read_watch: NULL\n");
-			continue;
-		}
-
-		sdisable = xs_read(xs, XBT_NULL, DISABLE_EXEC, 0);
-		if (sdisable)
-			goto next1;
-
-		if (strlen(vec[XS_WATCH_PATH]) < sizeof("state"))
-			goto next1;
-
-		/* find last component of path, check if it's "state" */
-		p = &vec[XS_WATCH_PATH][
-		    strlen(vec[XS_WATCH_PATH]) - sizeof("state")];
-		if (p[0] != '/')
-			goto next1;
-		p[0] = '\0';
-		p++;
-		if (strcmp(p, "state") != 0)
-			goto next1;
-
-		snprintf(buf, sizeof(buf), "%s/state", vec[XS_WATCH_PATH]);
-		sstate = xs_read(xs, XBT_NULL, buf, 0);
-		if (sstate == NULL) {
-			dolog(LOG_ERR,
-			    "Failed to read %s (%s)", buf, strerror(errno));
-			goto next1;
-		}
-
-		state = atoi(sstate);
-		snprintf(buf, sizeof(buf), "%s/hotplug-status",
-		    vec[XS_WATCH_PATH]);
-		s = xs_read(xs, XBT_NULL, buf, 0);
-		if (s != NULL && state != 6 /* XenbusStateClosed */)
-			goto next2;
-
-		type = DEVTYPE_UNKNOWN;
-		if (strncmp(vec[XS_WATCH_PATH],
-		    DOMAIN_PATH "/backend/vif",
-		    strlen(DOMAIN_PATH "/backend/vif")) == 0)
-			type = DEVTYPE_VIF;
-
-		if (strncmp(vec[XS_WATCH_PATH],
-		    DOMAIN_PATH "/backend/vbd",
-		    strlen(DOMAIN_PATH "/backend/vbd")) == 0)
-			type = DEVTYPE_VBD;
-
-		switch(type) {
-		case DEVTYPE_VIF:
-			free(s);
-			snprintf(buf, sizeof(buf), "%s/script",
-			    vec[XS_WATCH_PATH]);
-			s = xs_read(xs, XBT_NULL, buf, 0);
-			if (s == NULL) {
-				dolog(LOG_ERR,
-				    "Failed to read %s (%s)", buf,
-				    strerror(errno));
-				goto next2;
-			}
-			doexec(s, vec[XS_WATCH_PATH], sstate);
-			break;
-
-		case DEVTYPE_VBD:
-			doexec(vbd_script, vec[XS_WATCH_PATH], sstate);
-			break;
-
-		default:
-			break;
-		}
-
-next2:
-		free(s);
-		free(sstate);
-
-next1:
-		free(sdisable);
-		free(vec);
-	}
-
-	return 0;
-}
-- 
2.29.2

Re: [PATCH] NetBSD: remove xenbackendd

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:26PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> NetBSD doens't need xenbackendd with xl toolstack so don't build it.
> Remove now unused xenbackendd directory/files.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] NetBSD: remove xenbackendd

[Andrew Cooper]

On 15/01/2021 15:31, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:26PM +0100, Manuel Bouyer wrote:
>> From: Manuel Bouyer <bouyer@netbsd.org>
>>
>> NetBSD doens't need xenbackendd with xl toolstack so don't build it.
>> Remove now unused xenbackendd directory/files.
>>
>> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Thanks, Roger.

$ git grep backendd -- :/
../.gitignore:282:tools/xenbackendd/_paths.h
../.gitignore:283:tools/xenbackendd/xenbackendd
../tools/hotplug/Linux/init.d/sysconfig.xencommons.in:91:## Default: Not
defined, xenbackendd debug mode off
../tools/hotplug/Linux/init.d/sysconfig.xencommons.in:93:# Running
xenbackendd in debug mode
../tools/hotplug/NetBSD/block:4:# Called by xenbackendd
../tools/hotplug/NetBSD/rc.d/xencommons.in:25:XENBACKENDD_PIDFILE="@XEN_RUN_DIR@/xenbackendd.pid"
../tools/hotplug/NetBSD/vif-bridge:4:# Called by xenbackendd
../tools/hotplug/NetBSD/vif-ip:4:# Called by xenbackendd

I propose folding in the following deletions, if you're both happy?

~Andrew

diff --git a/.gitignore b/.gitignore
index 35957cc21f..9e0488e0cd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -279,8 +279,6 @@ tools/tests/vpci/vpci.[hc]
 tools/tests/vpci/test_vpci
 tools/xcutils/lsevtchn
 tools/xcutils/readnotes
-tools/xenbackendd/_paths.h
-tools/xenbackendd/xenbackendd
 tools/xenmon/xentrace_setmask
 tools/xenmon/xenbaked
 tools/xenpaging/xenpaging
diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index 0fc6557d4a..b059a2910d 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -87,12 +87,6 @@ XENSTORED_ARGS=
 # Only evaluated if XENSTORETYPE is "domain".
 XENSTORE_DOMAIN_ARGS=
 
-## Type: string
-## Default: Not defined, xenbackendd debug mode off
-#
-# Running xenbackendd in debug mode
-#XENBACKENDD_DEBUG=[yes|on|1]
-
 # qemu path
 #QEMU_XEN=@qemu_xen_path@
 
diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
index 32c20b6c89..2a0516f436 100644
--- a/tools/hotplug/NetBSD/block
+++ b/tools/hotplug/NetBSD/block
@@ -1,7 +1,6 @@
 #!/bin/sh -e
 
 # $NetBSD: block-nbsd,v 1.1.1.1 2008/08/07 20:26:57 cegger Exp $
-# Called by xenbackendd
 # Usage: block xsdir_backend_path state
 
 DIR=$(dirname "$0")
diff --git a/tools/hotplug/NetBSD/rc.d/xencommons.in
b/tools/hotplug/NetBSD/rc.d/xencommons.in
index 886a2e2840..3981787eac 100644
--- a/tools/hotplug/NetBSD/rc.d/xencommons.in
+++ b/tools/hotplug/NetBSD/rc.d/xencommons.in
@@ -22,8 +22,6 @@ required_files="/kern/xen/privcmd"
 
 XENSTORED_PIDFILE="@XEN_RUN_DIR@/xenstored.pid"
 XENCONSOLED_PIDFILE="@XEN_RUN_DIR@/xenconsoled.pid"
-XENBACKENDD_PIDFILE="@XEN_RUN_DIR@/xenbackendd.pid"
-#XENBACKENDD_DEBUG=1
 #XENCONSOLED_TRACE="@XEN_LOG_DIR@/xenconsole-trace.log"
 #XENSTORED_TRACE="@XEN_LOG_DIR@/xenstore-trace.log"
 
diff --git a/tools/hotplug/NetBSD/vif-bridge
b/tools/hotplug/NetBSD/vif-bridge
index b58e922601..b1b25cbbde 100644
--- a/tools/hotplug/NetBSD/vif-bridge
+++ b/tools/hotplug/NetBSD/vif-bridge
@@ -1,7 +1,6 @@
 #!/bin/sh -e
 
 # $NetBSD: vif-bridge-nbsd,v 1.1.1.1 2008/08/07 20:26:57 cegger Exp $
-# Called by xenbackendd
 # Usage: vif-bridge xsdir_backend_path state
 
 DIR=$(dirname "$0")
diff --git a/tools/hotplug/NetBSD/vif-ip b/tools/hotplug/NetBSD/vif-ip
index 83cbfe20e2..3e7bd2c022 100644
--- a/tools/hotplug/NetBSD/vif-ip
+++ b/tools/hotplug/NetBSD/vif-ip
@@ -1,7 +1,6 @@
 #!/bin/sh -e
 
 # $NetBSD: vif-ip-nbsd,v 1.1.1.1 2008/08/07 20:26:57 cegger Exp $
-# Called by xenbackendd
 # Usage: vif-ip xsdir_backend_path state
 
 DIR=$(dirname "$0")

Re: [PATCH] NetBSD: remove xenbackendd

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 06:31:45PM +0000, Andrew Cooper wrote:
> On 15/01/2021 15:31, Roger Pau Monné wrote:
> > On Tue, Jan 12, 2021 at 07:12:26PM +0100, Manuel Bouyer wrote:
> >> From: Manuel Bouyer <bouyer@netbsd.org>
> >>
> >> NetBSD doens't need xenbackendd with xl toolstack so don't build it.
> >> Remove now unused xenbackendd directory/files.
> >>
> >> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > Thanks, Roger.
> 
> $ git grep backendd -- :/
> ../.gitignore:282:tools/xenbackendd/_paths.h
> ../.gitignore:283:tools/xenbackendd/xenbackendd
> ../tools/hotplug/Linux/init.d/sysconfig.xencommons.in:91:## Default: Not
> defined, xenbackendd debug mode off
> ../tools/hotplug/Linux/init.d/sysconfig.xencommons.in:93:# Running
> xenbackendd in debug mode
> ../tools/hotplug/NetBSD/block:4:# Called by xenbackendd
> ../tools/hotplug/NetBSD/rc.d/xencommons.in:25:XENBACKENDD_PIDFILE="@XEN_RUN_DIR@/xenbackendd.pid"
> ../tools/hotplug/NetBSD/vif-bridge:4:# Called by xenbackendd
> ../tools/hotplug/NetBSD/vif-ip:4:# Called by xenbackendd
> 
> I propose folding in the following deletions, if you're both happy?

Sure, please do !

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] NetBSD: use system-provided headers

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD use the system-provided headers for ioctl and related definitions,
they are up to date and have more chances to match the kernel's idea of
the ioctls and structures.
Remove now-unused NetBSD/evtchn.h and NetBSD/privcmd.h.
Don't fail install if xen/sys/*.h are not present.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/debugger/gdbsx/xg/xg_main.c      |   4 +
 tools/include/Makefile                 |   2 +-
 tools/include/xen-sys/NetBSD/evtchn.h  |  86 --------------------
 tools/include/xen-sys/NetBSD/privcmd.h | 106 -------------------------
 tools/libs/call/private.h              |   4 +
 tools/libs/ctrl/xc_private.h           |   4 +
 tools/libs/foreignmemory/private.h     |   6 ++
 7 files changed, 19 insertions(+), 193 deletions(-)
 delete mode 100644 tools/include/xen-sys/NetBSD/evtchn.h
 delete mode 100644 tools/include/xen-sys/NetBSD/privcmd.h

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index a4e8653168..ce95648e7e 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -49,7 +49,11 @@
 #include "xg_public.h"
 #include <xen/version.h>
 #include <xen/domctl.h>
+#ifdef __NetBSD__
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 #include <xen/foreign/x86_32.h>
 #include <xen/foreign/x86_64.h>
 
diff --git a/tools/include/Makefile b/tools/include/Makefile
index 4d4ec5f974..5e90179e66 100644
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -68,7 +68,7 @@ install: all
 	$(INSTALL_DATA) xen/foreign/*.h $(DESTDIR)$(includedir)/xen/foreign
 	$(INSTALL_DATA) xen/hvm/*.h $(DESTDIR)$(includedir)/xen/hvm
 	$(INSTALL_DATA) xen/io/*.h $(DESTDIR)$(includedir)/xen/io
-	$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys
+	$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys || true
 	$(INSTALL_DATA) xen/xsm/*.h $(DESTDIR)$(includedir)/xen/xsm
 
 .PHONY: uninstall
diff --git a/tools/include/xen-sys/NetBSD/evtchn.h b/tools/include/xen-sys/NetBSD/evtchn.h
deleted file mode 100644
index 2d8a1f9164..0000000000
--- a/tools/include/xen-sys/NetBSD/evtchn.h
+++ /dev/null
@@ -1,86 +0,0 @@
-/* $NetBSD: evtchn.h,v 1.1.1.1 2007/06/14 19:39:45 bouyer Exp $ */
-/******************************************************************************
- * evtchn.h
- * 
- * Interface to /dev/xen/evtchn.
- * 
- * Copyright (c) 2003-2005, K A Fraser
- * 
- * This file may be distributed separately from the Linux kernel, or
- * incorporated into other software packages, subject to the following license:
- * 
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this source file (the "Software"), to deal in the Software without
- * restriction, including without limitation the rights to use, copy, modify,
- * merge, publish, distribute, sublicense, and/or sell copies of the Software,
- * and to permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- * 
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- * 
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- */
-
-#ifndef __NetBSD_EVTCHN_H__
-#define __NetBSD_EVTCHN_H__
-
-/*
- * Bind a fresh port to VIRQ @virq.
- */
-#define IOCTL_EVTCHN_BIND_VIRQ				\
-	_IOWR('E', 4, struct ioctl_evtchn_bind_virq)
-struct ioctl_evtchn_bind_virq {
-	unsigned int virq;
-	unsigned int port;
-};
-
-/*
- * Bind a fresh port to remote <@remote_domain, @remote_port>.
- */
-#define IOCTL_EVTCHN_BIND_INTERDOMAIN			\
-	_IOWR('E', 5, struct ioctl_evtchn_bind_interdomain)
-struct ioctl_evtchn_bind_interdomain {
-	unsigned int remote_domain, remote_port;
-	unsigned int port;
-};
-
-/*
- * Allocate a fresh port for binding to @remote_domain.
- */
-#define IOCTL_EVTCHN_BIND_UNBOUND_PORT			\
-	_IOWR('E', 6, struct ioctl_evtchn_bind_unbound_port)
-struct ioctl_evtchn_bind_unbound_port {
-	unsigned int remote_domain;
-	unsigned int port;
-};
-
-/*
- * Unbind previously allocated @port.
- */
-#define IOCTL_EVTCHN_UNBIND				\
-	_IOW('E', 7, struct ioctl_evtchn_unbind)
-struct ioctl_evtchn_unbind {
-	unsigned int port;
-};
-
-/*
- * Send event to previously allocated @port.
- */
-#define IOCTL_EVTCHN_NOTIFY				\
-	_IOW('E', 8, struct ioctl_evtchn_notify)
-struct ioctl_evtchn_notify {
-	unsigned int port;
-};
-
-/* Clear and reinitialise the event buffer. Clear error condition. */
-#define IOCTL_EVTCHN_RESET				\
-	_IO('E', 9)
-
-#endif /* __NetBSD_EVTCHN_H__ */
diff --git a/tools/include/xen-sys/NetBSD/privcmd.h b/tools/include/xen-sys/NetBSD/privcmd.h
deleted file mode 100644
index 555bad973e..0000000000
--- a/tools/include/xen-sys/NetBSD/privcmd.h
+++ /dev/null
@@ -1,106 +0,0 @@
-/*	NetBSD: xenio.h,v 1.3 2005/05/24 12:07:12 yamt Exp $	*/
-
-/******************************************************************************
- * privcmd.h
- * 
- * Copyright (c) 2003-2004, K A Fraser
- * 
- * This file may be distributed separately from the Linux kernel, or
- * incorporated into other software packages, subject to the following license:
- * 
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this source file (the "Software"), to deal in the Software without
- * restriction, including without limitation the rights to use, copy, modify,
- * merge, publish, distribute, sublicense, and/or sell copies of the Software,
- * and to permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- * 
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- * 
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- */
-
-#ifndef __NetBSD_PRIVCMD_H__
-#define __NetBSD_PRIVCMD_H__
-
-/* Interface to /dev/xen/privcmd */
-
-typedef struct privcmd_hypercall
-{
-    unsigned long op;
-    unsigned long arg[5];
-    long retval;
-} privcmd_hypercall_t;
-
-typedef struct privcmd_mmap_entry {
-    unsigned long va;
-    unsigned long mfn;
-    unsigned long npages;
-} privcmd_mmap_entry_t; 
-
-typedef struct privcmd_mmap {
-    int num;
-    domid_t dom; /* target domain */
-    privcmd_mmap_entry_t *entry;
-} privcmd_mmap_t; 
-
-typedef struct privcmd_mmapbatch {
-    int num;     /* number of pages to populate */
-    domid_t dom; /* target domain */
-    unsigned long addr;  /* virtual address */
-    unsigned long *arr; /* array of mfns - top nibble set on err */
-} privcmd_mmapbatch_t; 
-
-typedef struct privcmd_blkmsg
-{
-    unsigned long op;
-    void         *buf;
-    int           buf_size;
-} privcmd_blkmsg_t;
-
-/*
- * @cmd: IOCTL_PRIVCMD_HYPERCALL
- * @arg: &privcmd_hypercall_t
- * Return: Value returned from execution of the specified hypercall.
- */
-#define IOCTL_PRIVCMD_HYPERCALL         \
-    _IOWR('P', 0, privcmd_hypercall_t)
-
-#if defined(_KERNEL)
-/* compat */
-#define IOCTL_PRIVCMD_INITDOMAIN_EVTCHN_OLD \
-    _IO('P', 1)
-#endif /* defined(_KERNEL) */
-    
-#define IOCTL_PRIVCMD_MMAP             \
-    _IOW('P', 2, privcmd_mmap_t)
-#define IOCTL_PRIVCMD_MMAPBATCH        \
-    _IOW('P', 3, privcmd_mmapbatch_t)
-#define IOCTL_PRIVCMD_GET_MACH2PHYS_START_MFN \
-    _IOR('P', 4, unsigned long)
-
-/*
- * @cmd: IOCTL_PRIVCMD_INITDOMAIN_EVTCHN
- * @arg: n/a
- * Return: Port associated with domain-controller end of control event channel
- *         for the initial domain.
- */
-#define IOCTL_PRIVCMD_INITDOMAIN_EVTCHN \
-    _IOR('P', 5, int)
-
-/* Interface to /dev/xenevt */
-/* EVTCHN_RESET: Clear and reinit the event buffer. Clear error condition. */
-#define EVTCHN_RESET  _IO('E', 1)
-/* EVTCHN_BIND: Bind to the specified event-channel port. */
-#define EVTCHN_BIND   _IOW('E', 2, unsigned long)
-/* EVTCHN_UNBIND: Unbind from the specified event-channel port. */
-#define EVTCHN_UNBIND _IOW('E', 3, unsigned long)
-
-#endif /* __NetBSD_PRIVCMD_H__ */
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 21f992b37e..57e49356a1 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -7,7 +7,11 @@
 #include <xencall.h>
 
 #include <xen/xen.h>
+#ifdef __NetBSD__
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 
 #ifndef PAGE_SHIFT /* Mini-os, Yukk */
 #define PAGE_SHIFT           12
diff --git a/tools/libs/ctrl/xc_private.h b/tools/libs/ctrl/xc_private.h
index f0b5f83ac8..68e388f488 100644
--- a/tools/libs/ctrl/xc_private.h
+++ b/tools/libs/ctrl/xc_private.h
@@ -39,7 +39,11 @@
 #include <xenforeignmemory.h>
 #include <xendevicemodel.h>
 
+#ifdef __NetBSD__
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 
 #include <xen-tools/libs.h>
 
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 8f1bf081ed..b522a2b86b 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -8,7 +8,13 @@
 #include <xentoolcore_internal.h>
 
 #include <xen/xen.h>
+
+#ifdef __NetBSD__
+#include <xen/xen.h>
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 
 #ifndef PAGE_SHIFT /* Mini-os, Yukk */
 #define PAGE_SHIFT           12
-- 
2.29.2

Re: [PATCH] NetBSD: use system-provided headers

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:27PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> On NetBSD use the system-provided headers for ioctl and related definitions,
> they are up to date and have more chances to match the kernel's idea of
> the ioctls and structures.
> Remove now-unused NetBSD/evtchn.h and NetBSD/privcmd.h.
> Don't fail install if xen/sys/*.h are not present.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/debugger/gdbsx/xg/xg_main.c      |   4 +
>  tools/include/Makefile                 |   2 +-
>  tools/include/xen-sys/NetBSD/evtchn.h  |  86 --------------------
>  tools/include/xen-sys/NetBSD/privcmd.h | 106 -------------------------
>  tools/libs/call/private.h              |   4 +
>  tools/libs/ctrl/xc_private.h           |   4 +
>  tools/libs/foreignmemory/private.h     |   6 ++
>  7 files changed, 19 insertions(+), 193 deletions(-)
>  delete mode 100644 tools/include/xen-sys/NetBSD/evtchn.h
>  delete mode 100644 tools/include/xen-sys/NetBSD/privcmd.h
> 
> diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
> index a4e8653168..ce95648e7e 100644
> --- a/tools/debugger/gdbsx/xg/xg_main.c
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -49,7 +49,11 @@
>  #include "xg_public.h"
>  #include <xen/version.h>
>  #include <xen/domctl.h>
> +#ifdef __NetBSD__
> +#include <xen/xenio.h>
> +#else
>  #include <xen/sys/privcmd.h>
> +#endif
>  #include <xen/foreign/x86_32.h>
>  #include <xen/foreign/x86_64.h>
>  
> diff --git a/tools/include/Makefile b/tools/include/Makefile
> index 4d4ec5f974..5e90179e66 100644
> --- a/tools/include/Makefile
> +++ b/tools/include/Makefile
> @@ -68,7 +68,7 @@ install: all
>  	$(INSTALL_DATA) xen/foreign/*.h $(DESTDIR)$(includedir)/xen/foreign
>  	$(INSTALL_DATA) xen/hvm/*.h $(DESTDIR)$(includedir)/xen/hvm
>  	$(INSTALL_DATA) xen/io/*.h $(DESTDIR)$(includedir)/xen/io
> -	$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys
> +	$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys || true

This will mask real error on non-NetBSD OSes. My make-foo is very bad,
but maybe you could do something like:

if [ "$(XEN_OS)" != "NetBSD" ]; then \
	$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys; \
fi

Or maybe check whether the directory is not empty before attempting
the install?

Thanks, Roger.

[PATCH] gdbsx: use right path for privcmd

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD the privcmd interface node is /kern/xen/privcmd

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/debugger/gdbsx/xg/xg_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index ce95648e7e..83a009c195 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -130,11 +130,11 @@ xg_init()
     int flags, saved_errno;
 
     XGTRC("E\n");
-    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1) {
-        if ((_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1) {
-            perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd\n");
-            return -1;
-        }
+    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1 &&
+        (_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1 &&
+	(_dom0_fd=open("/kern/xen/privcmd", O_RDWR)) == -1) {
+        perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or /kern/xen/privcmd\n");
+        return -1;
     }
     /* Although we return the file handle as the 'xc handle' the API
      * does not specify / guarentee that this integer is in fact
-- 
2.29.2

Re: [PATCH] gdbsx: use right path for privcmd

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:28PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> On NetBSD the privcmd interface node is /kern/xen/privcmd
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/debugger/gdbsx/xg/xg_main.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
> index ce95648e7e..83a009c195 100644
> --- a/tools/debugger/gdbsx/xg/xg_main.c
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -130,11 +130,11 @@ xg_init()
>      int flags, saved_errno;
>  
>      XGTRC("E\n");
> -    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1) {
> -        if ((_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1) {
> -            perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd\n");
> -            return -1;
> -        }
> +    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1 &&
> +        (_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1 &&
> +	(_dom0_fd=open("/kern/xen/privcmd", O_RDWR)) == -1) {

Nit: hard tab instead of spaces.

> +        perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or /kern/xen/privcmd\n");

I would have split the line, so:

        perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or "
	       "/kern/xen/privcmd\n");

If you can resend with those fixed please add:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] gdbsx: use right path for privcmd

[Andrew Cooper]

On 18/01/2021 18:03, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:28PM +0100, Manuel Bouyer wrote:
>> From: Manuel Bouyer <bouyer@netbsd.org>
>>
>> On NetBSD the privcmd interface node is /kern/xen/privcmd
>>
>> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
>> ---
>>  tools/debugger/gdbsx/xg/xg_main.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
>> index ce95648e7e..83a009c195 100644
>> --- a/tools/debugger/gdbsx/xg/xg_main.c
>> +++ b/tools/debugger/gdbsx/xg/xg_main.c
>> @@ -130,11 +130,11 @@ xg_init()
>>      int flags, saved_errno;
>>  
>>      XGTRC("E\n");
>> -    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1) {
>> -        if ((_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1) {
>> -            perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd\n");
>> -            return -1;
>> -        }
>> +    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1 &&
>> +        (_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1 &&
>> +	(_dom0_fd=open("/kern/xen/privcmd", O_RDWR)) == -1) {
> Nit: hard tab instead of spaces.
>
>> +        perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or /kern/xen/privcmd\n");
> I would have split the line, so:
>
>         perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or "
> 	       "/kern/xen/privcmd\n");
>
> If you can resend with those fixed please add:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I'd shorten it to just "Failed to open privcmd".  I can fix on commit if
you're happy.

~Andrew

P.S. Part of me doesn't want to know why we're opencoding libxencall here...

Re: [PATCH] gdbsx: use right path for privcmd

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 06:45:42PM +0000, Andrew Cooper wrote:
> On 18/01/2021 18:03, Roger Pau Monné wrote:
> > On Tue, Jan 12, 2021 at 07:12:28PM +0100, Manuel Bouyer wrote:
> >> From: Manuel Bouyer <bouyer@netbsd.org>
> >>
> >> On NetBSD the privcmd interface node is /kern/xen/privcmd
> >>
> >> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> >> ---
> >>  tools/debugger/gdbsx/xg/xg_main.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
> >> index ce95648e7e..83a009c195 100644
> >> --- a/tools/debugger/gdbsx/xg/xg_main.c
> >> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> >> @@ -130,11 +130,11 @@ xg_init()
> >>      int flags, saved_errno;
> >>  
> >>      XGTRC("E\n");
> >> -    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1) {
> >> -        if ((_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1) {
> >> -            perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd\n");
> >> -            return -1;
> >> -        }
> >> +    if ((_dom0_fd=open("/dev/xen/privcmd", O_RDWR)) == -1 &&
> >> +        (_dom0_fd=open("/proc/xen/privcmd", O_RDWR)) == -1 &&
> >> +	(_dom0_fd=open("/kern/xen/privcmd", O_RDWR)) == -1) {
> > Nit: hard tab instead of spaces.
> >
> >> +        perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or /kern/xen/privcmd\n");
> > I would have split the line, so:
> >
> >         perror("Failed to open /dev/xen/privcmd or /proc/xen/privcmd or "
> > 	       "/kern/xen/privcmd\n");
> >
> > If you can resend with those fixed please add:
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I'd shorten it to just "Failed to open privcmd".  I can fix on commit if
> you're happy.

fine with me, thanks !

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] libs/call: fix build on NetBSD

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Define PAGE_* if not already defined
Catch up with osdep interface change.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/call/netbsd.c  | 19 +++++++++++--------
 tools/libs/call/private.h |  6 ++++--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/tools/libs/call/netbsd.c b/tools/libs/call/netbsd.c
index a5502da377..4dcc2919ba 100644
--- a/tools/libs/call/netbsd.c
+++ b/tools/libs/call/netbsd.c
@@ -19,12 +19,15 @@
  * Split from xc_netbsd.c
  */
 
-#include "xc_private.h"
 
 #include <unistd.h>
 #include <fcntl.h>
 #include <malloc.h>
+#include <errno.h>
 #include <sys/mman.h>
+#include <sys/ioctl.h>
+
+#include "private.h"
 
 int osdep_xencall_open(xencall_handle *xcall)
 {
@@ -69,12 +72,13 @@ int osdep_xencall_close(xencall_handle *xcall)
     return close(fd);
 }
 
-void *osdep_alloc_hypercall_buffer(xencall_handle *xcall, size_t npages)
+void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
 {
-    size_t size = npages * XC_PAGE_SIZE;
+    size_t size = npages * PAGE_SIZE;
     void *p;
+    int ret;
 
-    ret = posix_memalign(&p, XC_PAGE_SIZE, size);
+    ret = posix_memalign(&p, PAGE_SIZE, size);
     if ( ret != 0 || !p )
         return NULL;
 
@@ -86,14 +90,13 @@ void *osdep_alloc_hypercall_buffer(xencall_handle *xcall, size_t npages)
     return p;
 }
 
-void osdep_free_hypercall_buffer(xencall_handle *xcall, void *ptr,
-                                 size_t npages)
+void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
 {
-    (void) munlock(ptr, npages * XC_PAGE_SIZE);
+    munlock(ptr, npages * PAGE_SIZE);
     free(ptr);
 }
 
-int do_xen_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     int fd = xcall->fd;
     int error = ioctl(fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 57e49356a1..2ca84d723b 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -13,11 +13,13 @@
 #include <xen/sys/privcmd.h>
 #endif
 
-#ifndef PAGE_SHIFT /* Mini-os, Yukk */
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT           12
 #endif
-#ifndef __MINIOS__ /* Yukk */
+#ifndef PAGE_SIZE
 #define PAGE_SIZE            (1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK            (~(PAGE_SIZE-1))
 #endif
 
-- 
2.29.2

Re: [PATCH] libs/call: fix build on NetBSD

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:29PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Define PAGE_* if not already defined
> Catch up with osdep interface change.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one comment below, please keep the tag when resending with the
fixed comment.

> ---
>  tools/libs/call/netbsd.c  | 19 +++++++++++--------
>  tools/libs/call/private.h |  6 ++++--
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libs/call/netbsd.c b/tools/libs/call/netbsd.c
> index a5502da377..4dcc2919ba 100644
> --- a/tools/libs/call/netbsd.c
> +++ b/tools/libs/call/netbsd.c
> @@ -19,12 +19,15 @@
>   * Split from xc_netbsd.c
>   */
>  
> -#include "xc_private.h"
>  
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <malloc.h>
> +#include <errno.h>
>  #include <sys/mman.h>
> +#include <sys/ioctl.h>
> +
> +#include "private.h"
>  
>  int osdep_xencall_open(xencall_handle *xcall)
>  {
> @@ -69,12 +72,13 @@ int osdep_xencall_close(xencall_handle *xcall)
>      return close(fd);
>  }
>  
> -void *osdep_alloc_hypercall_buffer(xencall_handle *xcall, size_t npages)
> +void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
>  {
> -    size_t size = npages * XC_PAGE_SIZE;
> +    size_t size = npages * PAGE_SIZE;
>      void *p;
> +    int ret;
>  
> -    ret = posix_memalign(&p, XC_PAGE_SIZE, size);
> +    ret = posix_memalign(&p, PAGE_SIZE, size);
>      if ( ret != 0 || !p )
>          return NULL;
>  
> @@ -86,14 +90,13 @@ void *osdep_alloc_hypercall_buffer(xencall_handle *xcall, size_t npages)
>      return p;
>  }
>  
> -void osdep_free_hypercall_buffer(xencall_handle *xcall, void *ptr,
> -                                 size_t npages)
> +void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
>  {
> -    (void) munlock(ptr, npages * XC_PAGE_SIZE);
> +    munlock(ptr, npages * PAGE_SIZE);
>      free(ptr);
>  }
>  
> -int do_xen_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
> +int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
>  {
>      int fd = xcall->fd;
>      int error = ioctl(fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> index 57e49356a1..2ca84d723b 100644
> --- a/tools/libs/call/private.h
> +++ b/tools/libs/call/private.h
> @@ -13,11 +13,13 @@
>  #include <xen/sys/privcmd.h>
>  #endif
>  
> -#ifndef PAGE_SHIFT /* Mini-os, Yukk */
> +#ifndef PAGE_SHIFT

I would keep the comment somewhere that Mini-os already have all those
defined, and hence we can just straight define them.

Thanks, Roger.

[PATCH] libs/evtchn: fix build on NetBSD.

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

use xenio3.h for ioctl definitions
read_exact/write_exact seems to not be available here, which cause
a gcc error.
Use plain read/write, the xenevtchn interface won't do partial read/write
on NetBSD anyway so it should be safe.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Fixes: b7f76a699dc ('tools: Refactor /dev/xen/evtchn wrappers into libxenevtchn.
')

---
 tools/libs/evtchn/netbsd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libs/evtchn/netbsd.c b/tools/libs/evtchn/netbsd.c
index 8b8545d2f9..6d4ce28011 100644
--- a/tools/libs/evtchn/netbsd.c
+++ b/tools/libs/evtchn/netbsd.c
@@ -25,10 +25,10 @@
 
 #include <sys/ioctl.h>
 
-#include <xen/sys/evtchn.h>
-
 #include "private.h"
 
+#include <xen/xenio3.h>
+
 #define EVTCHN_DEV_NAME  "/dev/xenevt"
 
 int osdep_evtchn_open(xenevtchn_handle *xce)
@@ -131,7 +131,7 @@ xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
     int fd = xce->fd;
     evtchn_port_t port;
 
-    if ( read_exact(fd, (char *)&port, sizeof(port)) == -1 )
+    if ( read(fd, (char *)&port, sizeof(port)) == -1 )
         return -1;
 
     return port;
@@ -140,7 +140,7 @@ xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
 int xenevtchn_unmask(xenevtchn_handle *xce, evtchn_port_t port)
 {
     int fd = xce->fd;
-    return write_exact(fd, (char *)&port, sizeof(port));
+    return write(fd, (char *)&port, sizeof(port));
 }
 
 /*
-- 
2.29.2

Re: [PATCH] libs/evtchn: fix build on NetBSD.

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:30PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> use xenio3.h for ioctl definitions
> read_exact/write_exact seems to not be available here, which cause
> a gcc error.
> Use plain read/write, the xenevtchn interface won't do partial read/write
> on NetBSD anyway so it should be safe.

I would add: This is inline with the rest of the OS specific helpers
that also use plain read/write calls.

> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH] libs/foreignmemory: Implement on NetBSD

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Implement foreignmemory interface on NetBSD. The compat interface is now used
only on __sun__

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/foreignmemory/Makefile  |  2 +-
 tools/libs/foreignmemory/netbsd.c  | 75 ++++++++++++++++++++++++++----
 tools/libs/foreignmemory/private.h |  6 +--
 3 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index 13850f7988..f191cdbed0 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -8,7 +8,7 @@ SRCS-y                 += core.c
 SRCS-$(CONFIG_Linux)   += linux.c
 SRCS-$(CONFIG_FreeBSD) += freebsd.c
 SRCS-$(CONFIG_SunOS)   += compat.c solaris.c
-SRCS-$(CONFIG_NetBSD)  += compat.c netbsd.c
+SRCS-$(CONFIG_NetBSD)  += netbsd.c
 SRCS-$(CONFIG_MiniOS)  += minios.c
 
 include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
index 54a418ebd6..c2a691daed 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -19,7 +19,9 @@
 
 #include <unistd.h>
 #include <fcntl.h>
+#include <errno.h>
 #include <sys/mman.h>
+#include <sys/ioctl.h>
 
 #include "private.h"
 
@@ -66,15 +68,17 @@ int osdep_xenforeignmemory_close(xenforeignmemory_handle *fmem)
     return close(fd);
 }
 
-void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
-                              void *addr, int prot, int flags,
-                              xen_pfn_t *arr, int num)
+void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
+                                 uint32_t dom, void *addr,
+				 int prot, int flags, size_t num,
+				 const xen_pfn_t arr[/*num*/], int err[/*num*/])
+
 {
     int fd = fmem->fd;
-    privcmd_mmapbatch_t ioctlx;
-    addr = mmap(addr, num*XC_PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
+    privcmd_mmapbatch_v2_t ioctlx;
+    addr = mmap(addr, num*PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
     if ( addr == MAP_FAILED ) {
-        PERROR("osdep_map_foreign_batch: mmap failed");
+        PERROR("osdep_xenforeignmemory_map: mmap failed");
         return NULL;
     }
 
@@ -82,11 +86,12 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
     ioctlx.dom=dom;
     ioctlx.addr=(unsigned long)addr;
     ioctlx.arr=arr;
-    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx) < 0 )
+    ioctlx.err=err;
+    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx) < 0 )
     {
         int saved_errno = errno;
-        PERROR("osdep_map_foreign_batch: ioctl failed");
-        (void)munmap(addr, num*XC_PAGE_SIZE);
+        PERROR("osdep_xenforeignmemory_map: ioctl failed");
+        (void)munmap(addr, num*PAGE_SIZE);
         errno = saved_errno;
         return NULL;
     }
@@ -97,7 +102,57 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num)
 {
-    return munmap(addr, num*XC_PAGE_SIZE);
+    return munmap(addr, num*PAGE_SIZE);
+}
+
+int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
+                                    domid_t domid)
+{
+    errno = EOPNOTSUPP;
+    return -1;
+}
+
+int osdep_xenforeignmemory_unmap_resource(
+    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+}
+
+int osdep_xenforeignmemory_map_resource(
+    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+    privcmd_mmap_resource_t mr = {
+        .dom = fres->domid,
+        .type = fres->type,
+        .id = fres->id,
+        .idx = fres->frame,
+        .num = fres->nr_frames,
+    };
+    int rc;
+
+    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+                      fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
+    if ( fres->addr == MAP_FAILED )
+        return -1;
+
+    mr.addr = (uintptr_t)fres->addr;
+
+    rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
+    if ( rc )
+    {
+        int saved_errno;
+
+        if ( errno != EOPNOTSUPP )
+            PERROR("ioctl failed");
+
+        saved_errno = errno;
+        (void)osdep_xenforeignmemory_unmap_resource(fmem, fres);
+        errno = saved_errno;
+
+        return -1;
+    }
+
+    return 0;
 }
 
 /*
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index b522a2b86b..f7569d4f6b 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -42,9 +42,9 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num);
 
-#if defined(__NetBSD__) || defined(__sun__)
+#if defined(__sun__)
 /* Strictly compat for those two only only */
-void *compat_mapforeign_batch(xenforeignmem_handle *fmem, uint32_t dom,
+void *osdep_map_foreign_batch(xenforeignmemory_handle *fmem, uint32_t dom,
                               void *addr, int prot, int flags,
                               xen_pfn_t *arr, int num);
 #endif
@@ -60,7 +60,7 @@ struct xenforeignmemory_resource_handle {
     int flags;
 };
 
-#ifndef __linux__
+#if  !defined(__linux__) && !defined(__NetBSD__)
 static inline int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
                                                   domid_t domid)
 {
-- 
2.29.2

Re: [PATCH] libs/foreignmemory: Implement on NetBSD

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:31PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Implement foreignmemory interface on NetBSD. The compat interface is now used
> only on __sun__
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/libs/foreignmemory/Makefile  |  2 +-
>  tools/libs/foreignmemory/netbsd.c  | 75 ++++++++++++++++++++++++++----
>  tools/libs/foreignmemory/private.h |  6 +--
>  3 files changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
> index 13850f7988..f191cdbed0 100644
> --- a/tools/libs/foreignmemory/Makefile
> +++ b/tools/libs/foreignmemory/Makefile
> @@ -8,7 +8,7 @@ SRCS-y                 += core.c
>  SRCS-$(CONFIG_Linux)   += linux.c
>  SRCS-$(CONFIG_FreeBSD) += freebsd.c
>  SRCS-$(CONFIG_SunOS)   += compat.c solaris.c
> -SRCS-$(CONFIG_NetBSD)  += compat.c netbsd.c
> +SRCS-$(CONFIG_NetBSD)  += netbsd.c
>  SRCS-$(CONFIG_MiniOS)  += minios.c
>  
>  include $(XEN_ROOT)/tools/libs/libs.mk
> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> index 54a418ebd6..c2a691daed 100644
> --- a/tools/libs/foreignmemory/netbsd.c
> +++ b/tools/libs/foreignmemory/netbsd.c
> @@ -19,7 +19,9 @@
>  
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include <errno.h>
>  #include <sys/mman.h>
> +#include <sys/ioctl.h>
>  
>  #include "private.h"
>  
> @@ -66,15 +68,17 @@ int osdep_xenforeignmemory_close(xenforeignmemory_handle *fmem)
>      return close(fd);
>  }
>  
> -void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
> -                              void *addr, int prot, int flags,
> -                              xen_pfn_t *arr, int num)
> +void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
> +                                 uint32_t dom, void *addr,
> +				 int prot, int flags, size_t num,
> +				 const xen_pfn_t arr[/*num*/], int err[/*num*/])

Hard tabs in the last two lines added above.

> +
>  {
>      int fd = fmem->fd;
> -    privcmd_mmapbatch_t ioctlx;
> -    addr = mmap(addr, num*XC_PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
> +    privcmd_mmapbatch_v2_t ioctlx;

Nit: since you are already modifying this leaving an empty line here
would help readability IMO.

> +    addr = mmap(addr, num*PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
>      if ( addr == MAP_FAILED ) {
> -        PERROR("osdep_map_foreign_batch: mmap failed");
> +        PERROR("osdep_xenforeignmemory_map: mmap failed");
>          return NULL;
>      }
>  
> @@ -82,11 +86,12 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
>      ioctlx.dom=dom;
>      ioctlx.addr=(unsigned long)addr;
>      ioctlx.arr=arr;
> -    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx) < 0 )
> +    ioctlx.err=err;
> +    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx) < 0 )
>      {
>          int saved_errno = errno;
> -        PERROR("osdep_map_foreign_batch: ioctl failed");
> -        (void)munmap(addr, num*XC_PAGE_SIZE);
> +        PERROR("osdep_xenforeignmemory_map: ioctl failed");
> +        (void)munmap(addr, num*PAGE_SIZE);
>          errno = saved_errno;
>          return NULL;
>      }
> @@ -97,7 +102,57 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
>  int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
>                                   void *addr, size_t num)
>  {
> -    return munmap(addr, num*XC_PAGE_SIZE);
> +    return munmap(addr, num*PAGE_SIZE);
> +}
> +
> +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
> +                                    domid_t domid)
> +{
> +    errno = EOPNOTSUPP;
> +    return -1;
> +}
> +
> +int osdep_xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
> +}
> +
> +int osdep_xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    privcmd_mmap_resource_t mr = {
> +        .dom = fres->domid,
> +        .type = fres->type,
> +        .id = fres->id,
> +        .idx = fres->frame,
> +        .num = fres->nr_frames,
> +    };
> +    int rc;
> +
> +    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> +                      fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
> +    if ( fres->addr == MAP_FAILED )
> +        return -1;
> +
> +    mr.addr = (uintptr_t)fres->addr;
> +
> +    rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
> +    if ( rc )
> +    {
> +        int saved_errno;
> +
> +        if ( errno != EOPNOTSUPP )

Seems like NetBSD will return EINVAL for unknown privcmd ioctls?

http://bxr.su/NetBSD/sys/arch/xen/xen/privcmd.c#802

This would make differentiating between a real error or a missing
ioctl impossible AFAICT, so I think you can remove the check for
EOPNOTSUPP?

> +            PERROR("ioctl failed");
> +
> +        saved_errno = errno;
> +        (void)osdep_xenforeignmemory_unmap_resource(fmem, fres);
> +        errno = saved_errno;
> +
> +        return -1;
> +    }
> +
> +    return 0;
>  }
>  
>  /*
> diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
> index b522a2b86b..f7569d4f6b 100644
> --- a/tools/libs/foreignmemory/private.h
> +++ b/tools/libs/foreignmemory/private.h
> @@ -42,9 +42,9 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>  int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
>                                   void *addr, size_t num);
>  
> -#if defined(__NetBSD__) || defined(__sun__)
> +#if defined(__sun__)
>  /* Strictly compat for those two only only */
> -void *compat_mapforeign_batch(xenforeignmem_handle *fmem, uint32_t dom,
> +void *osdep_map_foreign_batch(xenforeignmemory_handle *fmem, uint32_t dom,
>                                void *addr, int prot, int flags,
>                                xen_pfn_t *arr, int num);
>  #endif
> @@ -60,7 +60,7 @@ struct xenforeignmemory_resource_handle {
>      int flags;
>  };
>  
> -#ifndef __linux__
> +#if  !defined(__linux__) && !defined(__NetBSD__)

Since I got the FreeBSD support committed, you would have to rebase
the series and this should then become:

#ifdef __sun__

I think. The rest of the patch LGTM.

Thanks, Roger.

[PATCH] libs/gnttab: implement on NetBSD

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Implement gnttab interface on NetBSD.
The kernel interface is different from FreeBSD so we can't use the FreeBSD
version

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/gnttab/Makefile |   2 +-
 tools/libs/gnttab/netbsd.c | 267 +++++++++++++++++++++++++++++++++++++
 2 files changed, 268 insertions(+), 1 deletion(-)
 create mode 100644 tools/libs/gnttab/netbsd.c

diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index d86c49d243..ae390ce60f 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -10,7 +10,7 @@ SRCS-GNTSHR            += gntshr_core.c
 SRCS-$(CONFIG_Linux)   += $(SRCS-GNTTAB) $(SRCS-GNTSHR) linux.c
 SRCS-$(CONFIG_MiniOS)  += $(SRCS-GNTTAB) gntshr_unimp.c minios.c
 SRCS-$(CONFIG_FreeBSD) += $(SRCS-GNTTAB) $(SRCS-GNTSHR) freebsd.c
+SRCS-$(CONFIG_NetBSD)  += $(SRCS-GNTTAB) $(SRCS-GNTSHR) netbsd.c
 SRCS-$(CONFIG_SunOS)   += gnttab_unimp.c gntshr_unimp.c
-SRCS-$(CONFIG_NetBSD)  += gnttab_unimp.c gntshr_unimp.c
 
 include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/gnttab/netbsd.c b/tools/libs/gnttab/netbsd.c
new file mode 100644
index 0000000000..2df7058cd7
--- /dev/null
+++ b/tools/libs/gnttab/netbsd.c
@@ -0,0 +1,267 @@
+/*
+ * Copyright (c) 2007-2008, D G Murray <Derek.Murray@cl.cam.ac.uk>
+ * Copyright (c) 2016-2017, Akshay Jaggi <jaggi@FreeBSD.org>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Split out from linux.c
+ */
+
+#include <fcntl.h>
+#include <errno.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+
+#include <xen/xen.h>
+#include <xen/xenio.h>
+
+#include "private.h"
+
+#define PAGE_SHIFT           12
+#define PAGE_SIZE            (1UL << PAGE_SHIFT)
+#define PAGE_MASK            (~(PAGE_SIZE-1))
+
+#define DEVXEN "/kern/xen/privcmd"
+
+int osdep_gnttab_open(xengnttab_handle *xgt)
+{
+    int fd = open(DEVXEN, O_RDWR|O_CLOEXEC);
+
+    if ( fd == -1 )
+        return -1;
+    xgt->fd = fd;
+
+    return 0;
+}
+
+int osdep_gnttab_close(xengnttab_handle *xgt)
+{
+    if ( xgt->fd == -1 )
+        return 0;
+
+    return close(xgt->fd);
+}
+
+int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
+{
+    return 0;
+}
+
+void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
+                             uint32_t count, int flags, int prot,
+                             uint32_t *domids, uint32_t *refs,
+                             uint32_t notify_offset,
+                             evtchn_port_t notify_port)
+{
+    uint32_t i;
+    int fd = xgt->fd;
+    struct ioctl_gntdev_mmap_grant_ref map;
+    void *addr = NULL;
+    int domids_stride;
+    unsigned int refs_size = count * sizeof(struct ioctl_gntdev_grant_ref);
+    int rv;
+
+    domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
+    map.refs = malloc(refs_size);
+
+    for ( i = 0; i < count; i++ )
+    {
+        map.refs[i].domid = domids[i * domids_stride];
+        map.refs[i].ref = refs[i];
+    }
+
+    map.count = count;
+    addr = mmap(NULL, count * PAGE_SIZE,
+	prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
+
+    if (map.va == MAP_FAILED) {
+        GTERROR(xgt->logger, "osdep_gnttab_grant_map: mmap failed");
+	munmap((void *)map.va, count * PAGE_SIZE);
+        addr = MAP_FAILED;
+    }
+    map.va = addr;
+
+    map.notify.offset = 0;
+    map.notify.action = 0;
+    if ( notify_offset < PAGE_SIZE * count )
+    {
+	map.notify.offset = notify_offset;
+	map.notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
+    }
+    if ( notify_port != -1 )
+    {
+       map.notify.event_channel_port = notify_port;
+       map.notify.action |= UNMAP_NOTIFY_SEND_EVENT;
+    }
+
+    rv = ioctl(fd, IOCTL_GNTDEV_MMAP_GRANT_REF, &map);
+    if ( rv )
+    {
+        GTERROR(xgt->logger,
+	    "ioctl IOCTL_GNTDEV_MMAP_GRANT_REF failed: %d", rv);
+        munmap(addr, count * PAGE_SIZE);
+        addr = MAP_FAILED;
+    }
+    free(map.refs);
+    return addr;
+}
+
+int osdep_gnttab_unmap(xengnttab_handle *xgt,
+                       void *start_address,
+                       uint32_t count)
+{
+    int rc;
+    if ( start_address == NULL )
+    {
+        errno = EINVAL;
+        return -1;
+    }
+
+    /* Next, unmap the memory. */
+    rc = munmap(start_address, count * PAGE_SIZE);
+
+    return rc;
+}
+
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+                            uint32_t count,
+                            xengnttab_grant_copy_segment_t *segs)
+{
+    errno = ENOSYS;
+    return -1;
+}
+
+int osdep_gntshr_open(xengntshr_handle *xgs)
+{
+
+    int fd = open(DEVXEN, O_RDWR);
+
+    if ( fd == -1 )
+        return -1;
+    xgs->fd = fd;
+
+    return 0;
+}
+
+int osdep_gntshr_close(xengntshr_handle *xgs)
+{
+    if ( xgs->fd == -1 )
+        return 0;
+
+    return close(xgs->fd);
+}
+
+void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
+                               uint32_t domid, int count,
+                               uint32_t *refs, int writable,
+                               uint32_t notify_offset,
+                               evtchn_port_t notify_port)
+{
+    int err;
+    int fd = xgs->fd;
+    void *area = NULL;
+    struct ioctl_gntdev_alloc_grant_ref alloc;
+
+    alloc.gref_ids = malloc(count * sizeof(uint32_t));
+    if ( alloc.gref_ids == NULL )
+        return NULL;
+    alloc.domid = domid;
+    alloc.flags = writable ? GNTDEV_ALLOC_FLAG_WRITABLE : 0;
+    alloc.count = count;
+    area = mmap(NULL, count * PAGE_SIZE,
+	PROT_READ | PROT_WRITE, MAP_ANON | MAP_SHARED, -1, 0);
+
+    if (area == MAP_FAILED) {
+        GTERROR(xgs->logger, "osdep_gnttab_grant_map: mmap failed");
+        area = MAP_FAILED;
+	goto out;
+    }
+    alloc.va = area;
+
+    alloc.notify.offset = 0;
+    alloc.notify.action = 0;
+    if ( notify_offset < PAGE_SIZE * count )
+    {
+	alloc.notify.offset = notify_offset;
+	alloc.notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
+    }
+    if ( notify_port != -1 )
+    {
+       alloc.notify.event_channel_port = notify_port;
+       alloc.notify.action |= UNMAP_NOTIFY_SEND_EVENT;
+    }
+    err = ioctl(fd, IOCTL_GNTDEV_ALLOC_GRANT_REF, &alloc);
+    if ( err )
+    {
+        GSERROR(xgs->logger, "IOCTL_GNTDEV_ALLOC_GRANT_REF failed");
+	munmap(area, count * PAGE_SIZE);
+	area = MAP_FAILED;
+        goto out;
+    }
+    memcpy(refs, alloc.gref_ids, count * sizeof(uint32_t));
+
+ out:
+    free(alloc.gref_ids);
+    return area;
+}
+
+int osdep_gntshr_unshare(xengntshr_handle *xgs,
+                         void *start_address, uint32_t count)
+{
+    return munmap(start_address, count * PAGE_SIZE);
+}
+
+/*
+ * The functions below are Linux-isms that will likely never be implemented
+ * on FreeBSD unless FreeBSD also implements something akin to Linux dmabuf.
+ */
+int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
+                                      uint32_t flags, uint32_t count,
+                                      const uint32_t *refs,
+                                      uint32_t *dmabuf_fd)
+{
+    abort();
+}
+
+int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
+                                          uint32_t fd, uint32_t wait_to_ms)
+{
+    abort();
+}
+
+int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
+                                    uint32_t fd, uint32_t count, uint32_t *refs)
+{
+    abort();
+}
+
+int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
+{
+    abort();
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.29.2

Re: [PATCH] libs/gnttab: implement on NetBSD

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:32PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Implement gnttab interface on NetBSD.
> The kernel interface is different from FreeBSD so we can't use the FreeBSD
> version

Since I'm not familiar with the NetBSD interface I can provide much
feedback, but you have some hard tabs in the code below which should
be removed.

Maybe you would like to be added as a maintainer for the tools NetBSD
files?

Thanks, Roger.

Re: [PATCH] libs/gnttab: implement on NetBSD

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 06:54:11PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:32PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > Implement gnttab interface on NetBSD.
> > The kernel interface is different from FreeBSD so we can't use the FreeBSD
> > version
> 
> Since I'm not familiar with the NetBSD interface I can provide much
> feedback, but you have some hard tabs in the code below which should
> be removed.
> 
> Maybe you would like to be added as a maintainer for the tools NetBSD
> files?

Yes, please.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/gnttab: implement on NetBSD

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 06:05:08PM +0100, Manuel Bouyer wrote:
> On Mon, Jan 18, 2021 at 06:54:11PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 12, 2021 at 07:12:32PM +0100, Manuel Bouyer wrote:
> > > From: Manuel Bouyer <bouyer@netbsd.org>
> > > 
> > > Implement gnttab interface on NetBSD.
> > > The kernel interface is different from FreeBSD so we can't use the FreeBSD
> > > version
> > 
> > Since I'm not familiar with the NetBSD interface I can provide much
> > feedback, but you have some hard tabs in the code below which should
> > be removed.
> > 
> > Maybe you would like to be added as a maintainer for the tools NetBSD
> > files?
> 
> Yes, please.

Could you send a patch to MAINTAINERS to that end? As a start just
searching for files/folders that have NetBSD in the name should give
you a list of paths to watch.

Thanks, Roger.

[PATCH] libs/light: Switch NetBSD to QEMU_XEN

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Switch NetBSD to QEMU_XEN.
All 3 versions of libxl__default_device_model() now return
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN, so remove it and just set
b_info->device_model_version to LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN in
libxl__domain_build_info_setdefault().

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/light/libxl_create.c   | 2 +-
 tools/libs/light/libxl_freebsd.c  | 5 -----
 tools/libs/light/libxl_internal.h | 2 --
 tools/libs/light/libxl_linux.c    | 5 -----
 tools/libs/light/libxl_netbsd.c   | 5 -----
 5 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 7199a79778..9848d65f36 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -102,7 +102,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
                 b_info->device_model_version =
                     LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
             } else {
-                b_info->device_model_version = libxl__default_device_model(gc);
+                b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
             }
         } else {
             b_info->device_model_version =
diff --git a/tools/libs/light/libxl_freebsd.c b/tools/libs/light/libxl_freebsd.c
index f7ef4a8910..422c6b3b79 100644
--- a/tools/libs/light/libxl_freebsd.c
+++ b/tools/libs/light/libxl_freebsd.c
@@ -229,11 +229,6 @@ out:
     return rc;
 }
 
-libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
-{
-    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
-}
-
 int libxl__pci_numdevs(libxl__gc *gc)
 {
     return ERROR_NI;
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index c79523ba92..6c8b7d71a9 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2309,8 +2309,6 @@ _hidden char *libxl__json_object_to_json(libxl__gc *gc,
   /* Based on /local/domain/$domid/dm-version xenstore key
    * default is qemu xen traditional */
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
-  /* Return the system-wide default device model */
-_hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
 
 static inline
 bool libxl__stubdomain_is_linux_running(libxl__gc *gc, uint32_t domid)
diff --git a/tools/libs/light/libxl_linux.c b/tools/libs/light/libxl_linux.c
index 873b0271af..8d62dfd255 100644
--- a/tools/libs/light/libxl_linux.c
+++ b/tools/libs/light/libxl_linux.c
@@ -241,11 +241,6 @@ out:
     return rc;
 }
 
-libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
-{
-    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
-}
-
 int libxl__pci_numdevs(libxl__gc *gc)
 {
     DIR *dir;
diff --git a/tools/libs/light/libxl_netbsd.c b/tools/libs/light/libxl_netbsd.c
index e66a393d7f..6ad4ed34c2 100644
--- a/tools/libs/light/libxl_netbsd.c
+++ b/tools/libs/light/libxl_netbsd.c
@@ -108,11 +108,6 @@ out:
     return rc;
 }
 
-libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
-{
-    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
-}
-
 int libxl__pci_numdevs(libxl__gc *gc)
 {
     return ERROR_NI;
-- 
2.29.2

Re: [PATCH] libs/light: Switch NetBSD to QEMU_XEN

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:33PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Switch NetBSD to QEMU_XEN.
> All 3 versions of libxl__default_device_model() now return
> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN, so remove it and just set
> b_info->device_model_version to LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN in
> libxl__domain_build_info_setdefault().
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH] libs/light: fix tv_sec printf format

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Don't assume tv_sec is a unsigned long, it is 64 bits on NetBSD 32 bits.
Use %jd and cast to (intmax_t) instead

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

---
 tools/libs/light/libxl_create.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 86f4a8369d..7199a79778 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -496,7 +496,7 @@ int libxl__domain_build(libxl__gc *gc,
         vments[2] = "image/ostype";
         vments[3] = "hvm";
         vments[4] = "start_time";
-        vments[5] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[5] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
 
         localents = libxl__calloc(gc, 13, sizeof(char *));
         i = 0;
@@ -535,7 +535,7 @@ int libxl__domain_build(libxl__gc *gc,
         vments[i++] = "image/kernel";
         vments[i++] = (char *) state->pv_kernel.path;
         vments[i++] = "start_time";
-        vments[i++] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[i++] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
         if (state->pv_ramdisk.path) {
             vments[i++] = "image/ramdisk";
             vments[i++] = (char *) state->pv_ramdisk.path;
@@ -1502,7 +1502,7 @@ static void domcreate_stream_done(libxl__egc *egc,
         vments[2] = "image/ostype";
         vments[3] = "hvm";
         vments[4] = "start_time";
-        vments[5] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[5] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         vments = libxl__calloc(gc, 11, sizeof(char *));
@@ -1512,7 +1512,7 @@ static void domcreate_stream_done(libxl__egc *egc,
         vments[i++] = "image/kernel";
         vments[i++] = (char *) state->pv_kernel.path;
         vments[i++] = "start_time";
-        vments[i++] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[i++] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
         if (state->pv_ramdisk.path) {
             vments[i++] = "image/ramdisk";
             vments[i++] = (char *) state->pv_ramdisk.path;
-- 
2.29.2

Re: [PATCH] libs/light: fix tv_sec printf format

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:34PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Don't assume tv_sec is a unsigned long, it is 64 bits on NetBSD 32 bits.
> Use %jd and cast to (intmax_t) instead
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH] libs/light: fix uuid on NetBSD

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

NetBSD uses the same uuid library as FreeBSD. As this is in a
__FreeBSD__ || __NetBSD__ block, just drop the #ifdef __FreeBSD__
and dead code.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/light/libxl_uuid.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/tools/libs/light/libxl_uuid.c b/tools/libs/light/libxl_uuid.c
index dadb79bad8..7b68270a33 100644
--- a/tools/libs/light/libxl_uuid.c
+++ b/tools/libs/light/libxl_uuid.c
@@ -82,7 +82,6 @@ void libxl_uuid_generate(libxl_uuid *uuid)
     uuid_enc_be(uuid->uuid, &nat_uuid);
 }
 
-#ifdef __FreeBSD__
 int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
 {
     uint32_t status;
@@ -95,19 +94,6 @@ int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
 
     return 0;
 }
-#else
-#define LIBXL__UUID_PTRS(uuid) &uuid[0], &uuid[1], &uuid[2], &uuid[3], \
-                               &uuid[4], &uuid[5], &uuid[6], &uuid[7], \
-                               &uuid[8], &uuid[9], &uuid[10],&uuid[11], \
-                               &uuid[12],&uuid[13],&uuid[14],&uuid[15]
-int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
-{
-    if ( sscanf(in, LIBXL_UUID_FMT, LIBXL__UUID_PTRS(uuid->uuid)) != sizeof(uuid->uuid) )
-        return -1;
-    return 0;
-}
-#undef LIBXL__UUID_PTRS
-#endif
 
 void libxl_uuid_copy(libxl_ctx *ctx_opt, libxl_uuid *dst,
                      const libxl_uuid *src)
@@ -120,7 +106,6 @@ void libxl_uuid_clear(libxl_uuid *uuid)
     memset(&uuid->uuid, 0, sizeof(uuid->uuid));
 }
 
-#ifdef __FreeBSD__
 int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
 {
     uuid_t nat_uuid1, nat_uuid2;
@@ -130,12 +115,6 @@ int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
 
     return uuid_compare(&nat_uuid1, &nat_uuid2, NULL);
 }
-#else
-int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
-{
-     return memcmp(uuid1->uuid, uuid2->uuid, sizeof(uuid1->uuid));
-}
-#endif
 
 const uint8_t *libxl_uuid_bytearray_const(const libxl_uuid *uuid)
 {
-- 
2.29.2

Re: [PATCH] libs/light: fix uuid on NetBSD

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:35PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> NetBSD uses the same uuid library as FreeBSD. As this is in a
> __FreeBSD__ || __NetBSD__ block, just drop the #ifdef __FreeBSD__
> and dead code.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH] libs/light: make it build without setresuid()

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

NetBSD doesn't have setresuid(). Add a configure check for it,
and use plain setuid() if !HAVE_SETRESUID

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/configure             | 13 +++++++++++++
 tools/configure.ac          |  3 +++
 tools/libs/light/libxl_dm.c | 10 ++++++++++
 3 files changed, 26 insertions(+)

diff --git a/tools/configure b/tools/configure
index 131112c41e..5e3793709e 100755
--- a/tools/configure
+++ b/tools/configure
@@ -9299,6 +9299,19 @@ _ACEOF
 
 esac
 
+# NetBSD doesnt have setresuid (yet)
+for ac_func in setresuid
+do :
+  ac_fn_c_check_func "$LINENO" "setresuid" "ac_cv_func_setresuid"
+if test "x$ac_cv_func_setresuid" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_SETRESUID 1
+_ACEOF
+
+fi
+done
+
+
 # Checks for header files.
 for ac_header in yajl/yajl_version.h sys/eventfd.h valgrind/memcheck.h utmp.h
 do :
diff --git a/tools/configure.ac b/tools/configure.ac
index ee8ba5ff24..04f78bf21d 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -457,6 +457,9 @@ AC_CHECK_DECLS([fdt_first_subnode, fdt_next_subnode],,,[#include <libfdt.h>])
 AC_CHECK_DECLS([fdt_property_u32],,,[#include <libfdt.h>])
 esac
 
+# NetBSD doesnt have setresuid (yet)
+AC_CHECK_FUNCS([setresuid])
+
 # Checks for header files.
 AC_CHECK_HEADERS([yajl/yajl_version.h sys/eventfd.h valgrind/memcheck.h utmp.h])
 
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 8866c3f5ad..7651429b9f 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -3653,6 +3653,7 @@ static int kill_device_model_uid_child(libxl__destroy_devicemodel_state *ddms,
     assert(reaper_uid);
     assert(dm_kill_uid);
 
+#if HAVE_SETRESUID
     LOGD(DEBUG, domid, "DM reaper: calling setresuid(%d, %d, 0)",
          reaper_uid, dm_kill_uid);
     r = setresuid(reaper_uid, dm_kill_uid, 0);
@@ -3662,6 +3663,15 @@ static int kill_device_model_uid_child(libxl__destroy_devicemodel_state *ddms,
         rc = rc ?: ERROR_FAIL;
         goto out;
     }
+#else /* HAVE_SETRESUID */
+    LOGD(DEBUG, domid, "DM reaper: calling setuid(%d)", dm_kill_uid);
+    r = setuid(dm_kill_uid);
+    if (r) {
+        LOGED(ERROR, domid, "setuid to %d", dm_kill_uid);
+        rc = rc ?: ERROR_FAIL;
+        goto out;
+    }
+#endif /* HAVE_SETRESUID */
 
     /*
      * And kill everyone but me.
-- 
2.29.2

Re: [PATCH] libs/light: make it build without setresuid()

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> NetBSD doesn't have setresuid(). Add a configure check for it,
> and use plain setuid() if !HAVE_SETRESUID
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

LGTM from a code PoV, but I think George/Ian should take a look, since
they know exactly what this is supposed to do, and I would bet there
are some reasons why setresuid is used instead of setuid, which should
likely be taken into account in the commit message to justify why
using setuid in it's place it's fine.

> ---
>  tools/configure             | 13 +++++++++++++
>  tools/configure.ac          |  3 +++
>  tools/libs/light/libxl_dm.c | 10 ++++++++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/tools/configure b/tools/configure
> index 131112c41e..5e3793709e 100755
> --- a/tools/configure
> +++ b/tools/configure
> @@ -9299,6 +9299,19 @@ _ACEOF
>  
>  esac
>  
> +# NetBSD doesnt have setresuid (yet)
> +for ac_func in setresuid
> +do :
> +  ac_fn_c_check_func "$LINENO" "setresuid" "ac_cv_func_setresuid"
> +if test "x$ac_cv_func_setresuid" = xyes; then :
> +  cat >>confdefs.h <<_ACEOF
> +#define HAVE_SETRESUID 1
> +_ACEOF
> +
> +fi
> +done
> +

We usually leave the changes to the generated configure script out of
the patch since it's usually a PITA to run the same autoconf version
in order to not generate tons of unrelated changes. You have managed
to run the same version, so I guess it's fine.

Just so that you know that you can ask commtters to re-run autoconf for
you by adding a note to the commit message to that effect.

Thanks, Roger.

Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > NetBSD doesn't have setresuid(). Add a configure check for it,
> > and use plain setuid() if !HAVE_SETRESUID
...
> LGTM from a code PoV, but I think George/Ian should take a look, since
> they know exactly what this is supposed to do, and I would bet there
> are some reasons why setresuid is used instead of setuid, which should
> likely be taken into account in the commit message to justify why
> using setuid in it's place it's fine.

There is indeed a reason for using setresuid here.  See the comments
at the top of kill_device_model_uid_child and the commit messages for
87f9458e3400 and 0c653574d39c.  This is all quite complex:

https://xenproject.org/2018/08/01/killing-processes-that-dont-want-to-be-killed/

https://marc.info/?l=xen-devel&m=152215770803468
 (search in that message for "libxl UID cleanup")

I wrote a message to George in 2018 proving that the desired set of
IDs cannot be made without setresuid.  I'll c&p the relevant part below.

I don't think setuid is safe - at least, if we are trying to restrict
the dm.  Since I think after the libxl child is forked, and has called
setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
the dm.  The dm could puppet it into pretending it had succeeded, but
then hang around until the domid is reused.

At the very least, this patch needs an argument, in detail, why this
is OK.

Also, why oh why does NetBSD not have setresuid ??  It's at least 20
years old !

Sorry,
Ian.

PS there is a long discussion of the history of saved set-ids, real vs
effective uids, etc., here
  https://pubs.opengroup.org/onlinepubs/9699919799/functions/setuid.html
but sadly it does not discuss setresuid.


From me to George etc. in 2018.

George emailed me a draft post:
> # No POSIX-compliant mousetraps?
> 
> Although `setresuid` is implemented by both Linux and FreeBSD, it is
> not in the [current POSIX
> specification](http://pubs.opengroup.org/onlinepubs/9699919799/).
> Looking at the official list of POSIX system interfaces, it's not
> clear how to get a process to have the required tuple using only POSIX
> interfaces (namely `setuid` and `setreuid`, without recourse to
> `setresuid` or Linux's `CAP_SETUID`); the assumption seems to be that
> `euid` must always be set to either `ruid` or `suid`.

Proof that this can't be simulated by proper use of setuid, seteuid
and setreuid:

					ruid	euid	suid

The desired state is:			reaper	target	reaper

If the final call is seteuid:

   seteuid(target);			reaper	target	reaper

For this to be permitted, and nontrivial, euid was 0:
   
Penultimate status			reaper	0	reaper

This state cannot be generated by setuid either euid==0 previously and
setuid would have set all of the ids; or the old euid was not 0, in
which case setuid() would have set only the euid, and required that
one of the other ids was 0, which can see that it can't have been.

This penultimate state cannot be generated by seteuid from any
different state.

So it must have been generated by setreuid.  We must avoid setreuid
setting the suid to the same as the new euid (0), which means that our
setreuid call did not change the ruid either.  That form of setreuid
is just like euid for our purposes, and not useful.

So the desired state could not be made by seteuid.

Let's consider setreuid.  Well, either setreuid sets the suid to the
same as the new euid, or it only changes the euid.  Ie, it would only
do something we could have done with seteuid and the argument above
applies.

What abouit setuid ?  Well, either setuid sets all three uids to the
same thing, or it, again, sets only the euid.

Ian.

Re: [PATCH] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Tue, Jan 12, 2021 at 07:12:36PM +0100, Manuel Bouyer wrote:
> > > From: Manuel Bouyer <bouyer@netbsd.org>
> > > 
> > > NetBSD doesn't have setresuid(). Add a configure check for it,
> > > and use plain setuid() if !HAVE_SETRESUID
> ...
> > LGTM from a code PoV, but I think George/Ian should take a look, since
> > they know exactly what this is supposed to do, and I would bet there
> > are some reasons why setresuid is used instead of setuid, which should
> > likely be taken into account in the commit message to justify why
> > using setuid in it's place it's fine.
> 
> There is indeed a reason for using setresuid here.  See the comments
> at the top of kill_device_model_uid_child and the commit messages for
> 87f9458e3400 and 0c653574d39c.  This is all quite complex:
> 
> https://xenproject.org/2018/08/01/killing-processes-that-dont-want-to-be-killed/
> 
> https://marc.info/?l=xen-devel&m=152215770803468
>  (search in that message for "libxl UID cleanup")
> 
> I wrote a message to George in 2018 proving that the desired set of
> IDs cannot be made without setresuid.  I'll c&p the relevant part below.
> 
> I don't think setuid is safe - at least, if we are trying to restrict
> the dm.  Since I think after the libxl child is forked, and has called

What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
so there isn't much to protect.

> setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
> the dm.  The dm could puppet it into pretending it had succeeded, but
> then hang around until the domid is reused.

I don't understand. We're talking about a simple kill(2) syscall here.

> 
> At the very least, this patch needs an argument, in detail, why this
> is OK.
> 
> Also, why oh why does NetBSD not have setresuid ??  It's at least 20
> years old !

It's not because it's old that it's good.



> 
> Sorry,

OK so if I understand properly, you say Xen should not be used on NetBSD ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson]

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> > I don't think setuid is safe - at least, if we are trying to restrict
> > the dm.  Since I think after the libxl child is forked, and has called
> 
> What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
> so there isn't much to protect.

Yes, the dm is qemu.  If qemu restriction is not supported, that makes
a big difference.  The complex situation here is to do with trying to
kill a possibly hostile qemu.

> > setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
> > the dm.  The dm could puppet it into pretending it had succeeded, but
> > then hang around until the domid is reused.
> 
> I don't understand. We're talking about a simple kill(2) syscall here.

If we're not trying to restrict qemu's privilege at all, then I think
the setuid is fine.  There are then only two remaining concerns I have
with this patch:

Firstly, we try to avoid #ifdefs like this.  It tends to make the code
rather tangled, especially over time.  Instead we prefer to move the
non-portable code into its own file, eg *_linux.c.

Secondly, I think we should check that dm_restrict is not enabled.
I think an assert would do since I think we believe this is already
prevented elsewhere ?

(One option for making this work would be to simply disable the
killing by uid on NetBSD.  But I don't think that's a good answer
because killing by uid after eg setuid is more reliable even if it is
not 100% bulletproof.  So switching to setuid or maybe setreuid is the
right answer.)

> OK so if I understand properly, you say Xen should not be used on NetBSD ?

I'm sorry to have offended and discouraged you.  That was not my
intention.  My apologies for sending an off-putting message.  For the
avoidance of any doubt, definitely don't think that.  We should make
this work properly.

Would you be willing to look into the two points I mention above and
send a revised version of the patch ?  If you find the refactoring
awkward I or Roger can help.

Regards,
Ian.

Re: [PATCH] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote:
> > > I don't think setuid is safe - at least, if we are trying to restrict
> > > the dm.  Since I think after the libxl child is forked, and has called
> > 
> > What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK,
> > so there isn't much to protect.
> 
> Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> a big difference.  The complex situation here is to do with trying to
> kill a possibly hostile qemu.

Hum, I'll have to check this (how to check, BTW ?).
I assumed qemu was running as root but it may not be completely true.
Especially as I notice, now that I'm re-reading the patch, that
we're doing a kill to -1. If we were doing so as root, user processes
would be killed.

> 
> > > setuid, it might be traceable (by NetBSD's equivalent of ptrace) by
> > > the dm.  The dm could puppet it into pretending it had succeeded, but
> > > then hang around until the domid is reused.
> > 
> > I don't understand. We're talking about a simple kill(2) syscall here.
> 
> If we're not trying to restrict qemu's privilege at all, then I think
> the setuid is fine.
> There are then only two remaining concerns I have
> with this patch:
> 
> Firstly, we try to avoid #ifdefs like this.  It tends to make the code
> rather tangled, especially over time.  Instead we prefer to move the
> non-portable code into its own file, eg *_linux.c.
> 
> Secondly, I think we should check that dm_restrict is not enabled.
> I think an assert would do since I think we believe this is already
> prevented elsewhere ?
> 
> (One option for making this work would be to simply disable the
> killing by uid on NetBSD.  But I don't think that's a good answer
> because killing by uid after eg setuid is more reliable even if it is
> not 100% bulletproof.  So switching to setuid or maybe setreuid is the
> right answer.)

This would have to be checked, but I don't think a non-root process
can ptrace a process whose saved-user-id is root.

Actually I think I could mimic the setresuid() with setreuid() and seteuid().

> 
> > OK so if I understand properly, you say Xen should not be used on NetBSD ?
> 
> I'm sorry to have offended and discouraged you.  That was not my
> intention.  My apologies for sending an off-putting message.  For the
> avoidance of any doubt, definitely don't think that.  We should make
> this work properly.
> 
> Would you be willing to look into the two points I mention above and
> send a revised version of the patch ?  If you find the refactoring
> awkward I or Roger can help.

Actually I don't see how I could split this in a different file, without
lot of duplicate code (even in just kill_device_model_uid_child(),
we're talking of about 7 lines of code out of 75). So some guidance here
would be welcome.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson]

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> > Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> > a big difference.  The complex situation here is to do with trying to
> > kill a possibly hostile qemu.
> 
> Hum, I'll have to check this (how to check, BTW ?).
> I assumed qemu was running as root but it may not be completely true.
> Especially as I notice, now that I'm re-reading the patch, that
> we're doing a kill to -1. If we were doing so as root, user processes
> would be killed.

It may well be that this whole piece of code won't be executed on
NetBSD becauwe dm restriction will be off.

The background: dm restriction is a set of arrangements for trying to
run qemu without given it any more privilege than it needs, and
certainly not ultimate privilege over the host.  This is quite
complicated and includes running it as a non-root user, chroot, and so
on.

On Linux it's run in its own network namespace, so that a qemu
compromised by the guest cannot access host daemons.  IDK what
facilities one might want to use on NetBSD to try to contain qemu.

This seems to me all a matter for future work.  I'm sorry that code
for a feature you're not going to be benefiting from is getting in
your way.

> > (One option for making this work would be to simply disable the
> > killing by uid on NetBSD.  But I don't think that's a good answer
> > because killing by uid after eg setuid is more reliable even if it is
> > not 100% bulletproof.  So switching to setuid or maybe setreuid is the
> > right answer.)
> 
> This would have to be checked, but I don't think a non-root process
> can ptrace a process whose saved-user-id is root.

If I remember rightly the saved-set-id is reset by setuid.  But I
could be wrong.  This stuff is all quite complex :-/.

> Actually I think I could mimic the setresuid() with setreuid() and seteuid().

My last mail had in it a thing that claims to be a proof that this is
not possible.

But I'm hoping we can avoid this.

> > > OK so if I understand properly, you say Xen should not be used on NetBSD ?
> > 
> > I'm sorry to have offended and discouraged you.  That was not my
> > intention.  My apologies for sending an off-putting message.  For the
> > avoidance of any doubt, definitely don't think that.  We should make
> > this work properly.
> > 
> > Would you be willing to look into the two points I mention above and
> > send a revised version of the patch ?  If you find the refactoring
> > awkward I or Roger can help.
> 
> Actually I don't see how I could split this in a different file, without
> lot of duplicate code (even in just kill_device_model_uid_child(),
> we're talking of about 7 lines of code out of 75). So some guidance here
> would be welcome.

I think splitting it out at precisely the function needed is probably
better.

Can you try this experiment: what happens if you replace the call to
setresuid with abort() ?  I think you may find it all works, because
you're not using that code path.

If so then I suggest introducing

  int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);

which would call setresuid on Linux and on NetBSD would do this

  assert(!"setresuid is not available on NetBSD, and dm restrction is not supported, so this code path should not have been reached")

What do you think ?

Ian.

Re: [PATCH] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote:
> > > Yes, the dm is qemu.  If qemu restriction is not supported, that makes
> > > a big difference.  The complex situation here is to do with trying to
> > > kill a possibly hostile qemu.
> > 
> > Hum, I'll have to check this (how to check, BTW ?).
> > I assumed qemu was running as root but it may not be completely true.
> > Especially as I notice, now that I'm re-reading the patch, that
> > we're doing a kill to -1. If we were doing so as root, user processes
> > would be killed.
> 
> It may well be that this whole piece of code won't be executed on
> NetBSD becauwe dm restriction will be off.
> 
> The background: dm restriction is a set of arrangements for trying to
> run qemu without given it any more privilege than it needs, and
> certainly not ultimate privilege over the host.  This is quite
> complicated and includes running it as a non-root user, chroot, and so
> on.
> 
> On Linux it's run in its own network namespace, so that a qemu
> compromised by the guest cannot access host daemons.  IDK what
> facilities one might want to use on NetBSD to try to contain qemu.
> 
> This seems to me all a matter for future work.  I'm sorry that code
> for a feature you're not going to be benefiting from is getting in
> your way.

On NetBSD we could start with a different uid and a chroot. This would
limit damages.

> > > right answer.)
> > 
> > This would have to be checked, but I don't think a non-root process
> > can ptrace a process whose saved-user-id is root.
> 
> If I remember rightly the saved-set-id is reset by setuid.  But I
> could be wrong.  This stuff is all quite complex :-/.

yes, setuid() resets the saved-user-id

> 
> > Actually I think I could mimic the setresuid() with setreuid() and seteuid().
> 
> My last mail had in it a thing that claims to be a proof that this is
> not possible.

This code:
        if (setreuid(375,0) < 0) {
                err(1, "setreuid");
        }
        if (seteuid(374) < 0) {
                err(1, "seteuid");
        }
        if (kill(-1, 9)) {
                err(1, "kill");
        }
        printf("kill done\n");
        if (seteuid(0) < 0) {
                err(1, "setreuid2");
        }
        exit(0);

actually works on NetBSD. processes from 375 are killed, and the
seteuid(0) call succeeds (showing that the saved used id is still 0).

> > 
> > Actually I don't see how I could split this in a different file, without
> > lot of duplicate code (even in just kill_device_model_uid_child(),
> > we're talking of about 7 lines of code out of 75). So some guidance here
> > would be welcome.
> 
> I think splitting it out at precisely the function needed is probably
> better.
> 
> Can you try this experiment: what happens if you replace the call to
> setresuid with abort() ?  I think you may find it all works, because
> you're not using that code path.
> 
> If so then I suggest introducing
> 
>   int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
> 
> which would call setresuid on Linux and on NetBSD would do this
> 
>   assert(!"setresuid is not available on NetBSD, and dm restrction is not supported, so this code path should not have been reached")
> 
> What do you think ?

As this is supported by Xen, I hope I can make at last run qemu with a
non-zero uid.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson]

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> > My last mail had in it a thing that claims to be a proof that this is
> > not possible.
> 
> This code:
>         if (setreuid(375,0) < 0) {
>                 err(1, "setreuid");
>         }
>         if (seteuid(374) < 0) {
>                 err(1, "seteuid");
>         }
>         if (kill(-1, 9)) {
>                 err(1, "kill");
>         }
>         printf("kill done\n");
>         if (seteuid(0) < 0) {
>                 err(1, "setreuid2");
>         }
>         exit(0);
> 
> actually works on NetBSD. processes from 375 are killed, and the
> seteuid(0) call succeeds (showing that the saved used id is still 0).

I guess I must have been wrong.

> > What do you think ?
> 
> As this is supported by Xen, I hope I can make at last run qemu with a
> non-zero uid.

The logic for deciding what user to run qemu as, and whether to kill
by uid or by pid, is in libxl_dm.c, in the function
libxl__domain_get_device_model_uid.

The dm_restrict flag turns on various other things too.

Ian.

Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> > > My last mail had in it a thing that claims to be a proof that this is
> > > not possible.
> > 
> > This code:
...
> > actually works on NetBSD. processes from 375 are killed, and the
> > seteuid(0) call succeeds (showing that the saved used id is still 0).
> 
> I guess I must have been wrong.
> 
> > > What do you think ?
> > 
> > As this is supported by Xen, I hope I can make at last run qemu with a
> > non-zero uid.
> 
> The logic for deciding what user to run qemu as, and whether to kill
> by uid or by pid, is in libxl_dm.c, in the function
> libxl__domain_get_device_model_uid.
> 
> The dm_restrict flag turns on various other things too.

I think I have lost track of where we are with this patch.  I would
like to see all this properly sorted in Xen 4.15.

How about I write a patch splitting the relevant part up into a
version for systems with setresuid and systems without ?  Then you
could fill in the missing part.

Should I expect the non-setresuid OS to provide effectively the whole
orf kill_device_model_uid_child, or just a replacement for the
setresuid call and surrounding logging, something like
  kill_device_model_uid_child_setresuid
?

Ian.

Re: [PATCH] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Wed, Jan 27, 2021 at 04:03:04PM +0000, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> > > On Wed, Jan 20, 2021 at 05:10:36PM +0000, Ian Jackson wrote:
> > > > My last mail had in it a thing that claims to be a proof that this is
> > > > not possible.
> > > 
> > > This code:
> ...
> > > actually works on NetBSD. processes from 375 are killed, and the
> > > seteuid(0) call succeeds (showing that the saved used id is still 0).
> > 
> > I guess I must have been wrong.
> > 
> > > > What do you think ?
> > > 
> > > As this is supported by Xen, I hope I can make at last run qemu with a
> > > non-zero uid.
> > 
> > The logic for deciding what user to run qemu as, and whether to kill
> > by uid or by pid, is in libxl_dm.c, in the function
> > libxl__domain_get_device_model_uid.
> > 
> > The dm_restrict flag turns on various other things too.
> 
> I think I have lost track of where we are with this patch.  I would
> like to see all this properly sorted in Xen 4.15.
> 
> How about I write a patch splitting the relevant part up into a
> version for systems with setresuid and systems without ?  Then you
> could fill in the missing part.

Yesterday I sent a v2 with the rewriting you suggested. But I'm fine
with you doing the rewrite.

> 
> Should I expect the non-setresuid OS to provide effectively the whole
> orf kill_device_model_uid_child, or just a replacement for the
> setresuid call and surrounding logging, something like
>   kill_device_model_uid_child_setresuid

As far as I'm concerned, kill_device_model_uid_child_setresuid() is enough.

Unfortunably I don't think I'll have time to work on dm restriction
for NetBSD before 4.15

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: make it build without setresuid()

[Ian Jackson]

Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without setresuid()"):
> On Wed, Jan 27, 2021 at 04:03:04PM +0000, Ian Jackson wrote:
> > How about I write a patch splitting the relevant part up into a
> > version for systems with setresuid and systems without ?  Then you
> > could fill in the missing part.
> 
> Yesterday I sent a v2 with the rewriting you suggested. But I'm fine
> with you doing the rewrite.

OK.

> > Should I expect the non-setresuid OS to provide effectively the whole
> > orf kill_device_model_uid_child, or just a replacement for the
> > setresuid call and surrounding logging, something like
> >   kill_device_model_uid_child_setresuid
> 
> As far as I'm concerned, kill_device_model_uid_child_setresuid() is enough.
> 
> Unfortunably I don't think I'll have time to work on dm restriction
> for NetBSD before 4.15

No problem.


Taking a step back, I think this series is very close to going in, if
not actually ready.  Do you have a git branch version of this ?

I find the series of un-numbered patches you have posted make it hard
to see the wood for the trees, and collect all the relevant pieces.  A
git branch version would make the life of the committer considerably
easier.

If you don't have a favourite public git server you can post me a link
to, you could use gitlab.com where we have a mirror of xen.git.
   https://gitlab.com/xen-project/xen

(If you need instructions mail me privately.)

Regards,
Ian.

Re: [PATCH] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 11:39:03AM +0000, Ian Jackson wrote:
> [...]
> Taking a step back, I think this series is very close to going in, if
> not actually ready.  Do you have a git branch version of this ?

Actually no. I'm not used to git, and I find it quite hard to use
(and is a large part of the time I spend to get the serie of patches ready).

The only thing I'm doing right now it a git clone from the xen repo,
commit my patches and do a git format-patch
When doing a new version that need code change I start again from scratch,
because I didn't find how to do incremental commits and then
have git format-patch output something sensible.

I nerver understood how git branches and merge were supposed to work
(for example syncing a local repo with upstream, while keeping local
changes).

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] libs/light: pass some infos to qemu

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

Pass bridge name to qemu as command line option
When starting qemu, set an environnement variable XEN_DOMAIN_ID,
to be used by qemu helper scripts

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/light/libxl_dm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 3da83259c0..8866c3f5ad 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
         int nr_set_cpus = 0;
         char *s;
 
+        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid));
+
         if (b_info->kernel) {
             LOGD(ERROR, domid, "HVM direct kernel boot is not supported by "
                  "qemu-xen-traditional");
@@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 flexarray_append(dm_args, "-netdev");
                 flexarray_append(dm_args,
                                  GCSPRINTF("type=tap,id=net%d,ifname=%s,"
+					   "br=%s,"
                                            "script=%s,downscript=%s",
                                            nics[i].devid, ifname,
+					   nics[i].bridge,
                                            libxl_tapif_script(gc),
                                            libxl_tapif_script(gc)));
 
@@ -1825,6 +1829,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_append(dm_args, GCSPRINTF("%"PRId64, ram_size));
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", guest_domid));
+
         if (b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI)
             flexarray_append_pair(dm_args, "-device", "ahci,id=ahci0");
         for (i = 0; i < num_disks; i++) {
-- 
2.29.2

Re: [PATCH] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Pass bridge name to qemu as command line option
> When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> to be used by qemu helper scripts
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/libs/light/libxl_dm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> index 3da83259c0..8866c3f5ad 100644
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
>          int nr_set_cpus = 0;
>          char *s;
>  
> +        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid));
> +
>          if (b_info->kernel) {
>              LOGD(ERROR, domid, "HVM direct kernel boot is not supported by "
>                   "qemu-xen-traditional");
> @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                  flexarray_append(dm_args, "-netdev");
>                  flexarray_append(dm_args,
>                                   GCSPRINTF("type=tap,id=net%d,ifname=%s,"
> +					   "br=%s,"
>                                             "script=%s,downscript=%s",
>                                             nics[i].devid, ifname,
> +					   nics[i].bridge,

You have some hard tabs in there.

Also looking at the manual the br= option seems to only be available
for the bridge networking mode, while here Xen is using tap instead?

Thanks, Roger.

Re: [PATCH] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote:
> On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > Pass bridge name to qemu as command line option
> > When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> > to be used by qemu helper scripts
> > 
> > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > ---
> >  tools/libs/light/libxl_dm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > index 3da83259c0..8866c3f5ad 100644
> > --- a/tools/libs/light/libxl_dm.c
> > +++ b/tools/libs/light/libxl_dm.c
> > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
> >          int nr_set_cpus = 0;
> >          char *s;
> >  
> > +        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid));
> > +
> >          if (b_info->kernel) {
> >              LOGD(ERROR, domid, "HVM direct kernel boot is not supported by "
> >                   "qemu-xen-traditional");
> > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >                  flexarray_append(dm_args, "-netdev");
> >                  flexarray_append(dm_args,
> >                                   GCSPRINTF("type=tap,id=net%d,ifname=%s,"
> > +					   "br=%s,"
> >                                             "script=%s,downscript=%s",
> >                                             nics[i].devid, ifname,
> > +					   nics[i].bridge,
> 
> You have some hard tabs in there.

Yes. What's the problem ?

> 
> Also looking at the manual the br= option seems to only be available
> for the bridge networking mode, while here Xen is using tap instead?

Unless I missed something, the bridge networking mode is using the
tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup
script is doing
exec /sbin/brconfig $2 add $1

(the script is called with: qemu-ifup <tap if> <bridge if>)

This is a problem that hit me when I converted NetBSD to qemu-xen:
qemu-traditional does call the qemu-ifup script with the 2 parameters,
while qemu-xen calls it only with the tap if. So the qemu-ifup script can't
know to which bridge the tap interface should be attached to.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Sat, Jan 16, 2021 at 12:25:02PM +0100, Manuel Bouyer wrote:
> On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote:
> > > From: Manuel Bouyer <bouyer@netbsd.org>
> > > 
> > > Pass bridge name to qemu as command line option
> > > When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> > > to be used by qemu helper scripts
> > > 
> > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > > ---
> > >  tools/libs/light/libxl_dm.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > > index 3da83259c0..8866c3f5ad 100644
> > > --- a/tools/libs/light/libxl_dm.c
> > > +++ b/tools/libs/light/libxl_dm.c
> > > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
> > >          int nr_set_cpus = 0;
> > >          char *s;
> > >  
> > > +        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid));
> > > +
> > >          if (b_info->kernel) {
> > >              LOGD(ERROR, domid, "HVM direct kernel boot is not supported by "
> > >                   "qemu-xen-traditional");
> > > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> > >                  flexarray_append(dm_args, "-netdev");
> > >                  flexarray_append(dm_args,
> > >                                   GCSPRINTF("type=tap,id=net%d,ifname=%s,"
> > > +					   "br=%s,"
> > >                                             "script=%s,downscript=%s",
> > >                                             nics[i].devid, ifname,
> > > +					   nics[i].bridge,
> > 
> > You have some hard tabs in there.
> 
> Yes. What's the problem ?

This file (and libxenlight) uses only spaces for indentation, it
breaks the coding style.

The line you added above uses spaces and it's fine.

> > 
> > Also looking at the manual the br= option seems to only be available
> > for the bridge networking mode, while here Xen is using tap instead?
> 
> Unless I missed something, the bridge networking mode is using the
> tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup
> script is doing
> exec /sbin/brconfig $2 add $1
> 
> (the script is called with: qemu-ifup <tap if> <bridge if>)
> 
> This is a problem that hit me when I converted NetBSD to qemu-xen:
> qemu-traditional does call the qemu-ifup script with the 2 parameters,
> while qemu-xen calls it only with the tap if. So the qemu-ifup script can't
> know to which bridge the tap interface should be attached to.

OK, so the only functional difference of adding the br parameter is
that it gets passed to the script. I would add that to the commit
message:

"The only functional difference of using the br parameter is that the
bridge name gets passed to the QEMU script."

Note also that there are networking modes that don't use a bridge, so
you could likely end up with nics[i].bridge == NULL?

I also wonder why NetBSD needs to add the tap interface to the bridge
in the QEMU script instead of doing it from the hotplug script called
by libxl, like Linux and FreeBSD do.

Thanks, Roger.

Re: [PATCH] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote:
> On Sat, Jan 16, 2021 at 12:25:02PM +0100, Manuel Bouyer wrote:
> > On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote:
> > > On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote:
> > > > From: Manuel Bouyer <bouyer@netbsd.org>
> > > > 
> > > > Pass bridge name to qemu as command line option
> > > > When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> > > > to be used by qemu helper scripts
> > > > 
> > > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > > > ---
> > > >  tools/libs/light/libxl_dm.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > > > index 3da83259c0..8866c3f5ad 100644
> > > > --- a/tools/libs/light/libxl_dm.c
> > > > +++ b/tools/libs/light/libxl_dm.c
> > > > @@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
> > > >          int nr_set_cpus = 0;
> > > >          char *s;
> > > >  
> > > > +        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid));
> > > > +
> > > >          if (b_info->kernel) {
> > > >              LOGD(ERROR, domid, "HVM direct kernel boot is not supported by "
> > > >                   "qemu-xen-traditional");
> > > > @@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> > > >                  flexarray_append(dm_args, "-netdev");
> > > >                  flexarray_append(dm_args,
> > > >                                   GCSPRINTF("type=tap,id=net%d,ifname=%s,"
> > > > +					   "br=%s,"
> > > >                                             "script=%s,downscript=%s",
> > > >                                             nics[i].devid, ifname,
> > > > +					   nics[i].bridge,
> > > 
> > > You have some hard tabs in there.
> > 
> > Yes. What's the problem ?
> 
> This file (and libxenlight) uses only spaces for indentation, it
> breaks the coding style.
> 
> The line you added above uses spaces and it's fine.

Ha OK, will fix. I didn't see a difference in my editor.

> 
> > > 
> > > Also looking at the manual the br= option seems to only be available
> > > for the bridge networking mode, while here Xen is using tap instead?
> > 
> > Unless I missed something, the bridge networking mode is using the
> > tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup
> > script is doing
> > exec /sbin/brconfig $2 add $1
> > 
> > (the script is called with: qemu-ifup <tap if> <bridge if>)
> > 
> > This is a problem that hit me when I converted NetBSD to qemu-xen:
> > qemu-traditional does call the qemu-ifup script with the 2 parameters,
> > while qemu-xen calls it only with the tap if. So the qemu-ifup script can't
> > know to which bridge the tap interface should be attached to.
> 
> OK, so the only functional difference of adding the br parameter is
> that it gets passed to the script. I would add that to the commit
> message:
> 
> "The only functional difference of using the br parameter is that the
> bridge name gets passed to the QEMU script."

will do.

> 
> Note also that there are networking modes that don't use a bridge, so
> you could likely end up with nics[i].bridge == NULL?

I guess it would be nics[i].bridge="" (or some other default value) but
I will check. AFAIK qemu will always be called with nics tap mode ?

> 
> I also wonder why NetBSD needs to add the tap interface to the bridge
> in the QEMU script instead of doing it from the hotplug script called
> by libxl, like Linux and FreeBSD do.

the tap interface is created by qemu itself, its name is not known outside
of qemu. Also, is there guarantee that qemu has created the tap before
the hotplug script is called ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Mon, Jan 18, 2021 at 09:52:14AM +0100, Manuel Bouyer wrote:
> On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote:
> > I also wonder why NetBSD needs to add the tap interface to the bridge
> > in the QEMU script instead of doing it from the hotplug script called
> > by libxl, like Linux and FreeBSD do.
> 
> the tap interface is created by qemu itself, its name is not known outside
> of qemu. Also, is there guarantee that qemu has created the tap before
> the hotplug script is called ?

Yes, the toolstack will wait for QEMU to be initialized at which point
the tap interface has been created.

I think I remember now why this didn't work on NetBSD. We ask QEMU to
create the tap interface with a specific name (using the vifname=
parameter), but NetBSD doesn't have the ioctl to rename network
interfaces implemented, and thus cannot rename the interface from tapX
to vifX.Y-emu, and hence you need to use the QEMU script from QEMU
itself because it's the only entity that knows the name of the tap
interface created.

If you agree, can you add something along the lines to the commit
message? So that we remember why NetBSD needs to use the QEMU scripts.

Thanks, Roger.

Re: [PATCH] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 10:07:22AM +0100, Roger Pau Monné wrote:
> On Mon, Jan 18, 2021 at 09:52:14AM +0100, Manuel Bouyer wrote:
> > On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote:
> > > I also wonder why NetBSD needs to add the tap interface to the bridge
> > > in the QEMU script instead of doing it from the hotplug script called
> > > by libxl, like Linux and FreeBSD do.
> > 
> > the tap interface is created by qemu itself, its name is not known outside
> > of qemu. Also, is there guarantee that qemu has created the tap before
> > the hotplug script is called ?
> 
> Yes, the toolstack will wait for QEMU to be initialized at which point
> the tap interface has been created.
> 
> I think I remember now why this didn't work on NetBSD. We ask QEMU to
> create the tap interface with a specific name (using the vifname=
> parameter), but NetBSD doesn't have the ioctl to rename network
> interfaces implemented, and thus cannot rename the interface from tapX
> to vifX.Y-emu, and hence you need to use the QEMU script from QEMU
> itself because it's the only entity that knows the name of the tap
> interface created.

Yes; at some point I proposed to support name aliases to interface
name in NetBSD but it got rejected ... so we have to use the
name the kernel did choose for us.

> 
> If you agree, can you add something along the lines to the commit
> message? So that we remember why NetBSD needs to use the QEMU scripts.

Will do.
thanks 

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote:
> [...]
> 
> Note also that there are networking modes that don't use a bridge, so
> you could likely end up with nics[i].bridge == NULL?

I couldn't cause this to happen. If no bridge is specified in the
domU's config file, qemu-ifup is called with xenbr0 as bridge name.

I tried this:
vif = [ 'mac=00:16:3e:00:10:54, gatewaydev=wm0 type=ioemu, model=e1000' ]

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:42:23PM +0100, Manuel Bouyer wrote:
> On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote:
> > [...]
> > 
> > Note also that there are networking modes that don't use a bridge, so
> > you could likely end up with nics[i].bridge == NULL?
> 
> I couldn't cause this to happen. If no bridge is specified in the
> domU's config file, qemu-ifup is called with xenbr0 as bridge name.
> 
> I tried this:
> vif = [ 'mac=00:16:3e:00:10:54, gatewaydev=wm0 type=ioemu, model=e1000' ]

Right, that's because libxl__device_nic_setdefault will set the bridge
field to xenbr0 if empty.

I'm not opposed to this behavior, seeing as we don't have much other
option I'm afraid. I guess we could open the tap interface in libxl
and pass it to QEMU, so that libxl can call the hotplug script knowing
the tap interface name, but that seems non-trivial.

IMO if we go that route it needs to be documented that NetBSD only
supports bridged mode for emulated HVM network interfaces for the time
being.

Thanks, Roger.

Re: [PATCH] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Wed, Jan 27, 2021 at 10:06:43AM +0100, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 11:42:23PM +0100, Manuel Bouyer wrote:
> > On Mon, Jan 18, 2021 at 09:36:42AM +0100, Roger Pau Monné wrote:
> > > [...]
> > > 
> > > Note also that there are networking modes that don't use a bridge, so
> > > you could likely end up with nics[i].bridge == NULL?
> > 
> > I couldn't cause this to happen. If no bridge is specified in the
> > domU's config file, qemu-ifup is called with xenbr0 as bridge name.
> > 
> > I tried this:
> > vif = [ 'mac=00:16:3e:00:10:54, gatewaydev=wm0 type=ioemu, model=e1000' ]
> 
> Right, that's because libxl__device_nic_setdefault will set the bridge
> field to xenbr0 if empty.
> 
> I'm not opposed to this behavior, seeing as we don't have much other
> option I'm afraid. I guess we could open the tap interface in libxl
> and pass it to QEMU, so that libxl can call the hotplug script knowing
> the tap interface name, but that seems non-trivial.
> 
> IMO if we go that route it needs to be documented that NetBSD only
> supports bridged mode for emulated HVM network interfaces for the time
> being.

that's not quite true. As long as you have the domain ID in the qemu-if script,
you can query parameters from the store and do whatever you want here
(for example I use it to setup firewall rules). 

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] libs/store: make build without PTHREAD_STACK_MIN

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD, PTHREAD_STACK_MIN is not available.
Just use DEFAULT_THREAD_STACKSIZE if PTHREAD_STACK_MIN is not available.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/store/xs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 4ac73ec317..8e646b98d6 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -811,9 +811,13 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
 
 #ifdef USE_PTHREAD
 #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
+#ifndef PTHREAD_STACK_MIN
+#define READ_THREAD_STACKSIZE DEFAULT_THREAD_STACKSIZE
+#else
 #define READ_THREAD_STACKSIZE 					\
 	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
 	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
+#endif
 
 	/* We dynamically create a reader thread on demand. */
 	mutex_lock(&h->request_mutex);
-- 
2.29.2

Re: [PATCH] libs/store: make build without PTHREAD_STACK_MIN

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:38PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> On NetBSD, PTHREAD_STACK_MIN is not available.
> Just use DEFAULT_THREAD_STACKSIZE if PTHREAD_STACK_MIN is not available.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] libs/store: make build without PTHREAD_STACK_MIN

[Andrew Cooper]

On 12/01/2021 18:12, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
>
> On NetBSD, PTHREAD_STACK_MIN is not available.
> Just use DEFAULT_THREAD_STACKSIZE if PTHREAD_STACK_MIN is not available.
>
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/libs/store/xs.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 4ac73ec317..8e646b98d6 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -811,9 +811,13 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
>  
>  #ifdef USE_PTHREAD
>  #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
> +#ifndef PTHREAD_STACK_MIN
> +#define READ_THREAD_STACKSIZE DEFAULT_THREAD_STACKSIZE
> +#else
>  #define READ_THREAD_STACKSIZE 					\
>  	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
>  	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
> +#endif

How about this:

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 4ac73ec317..3fa3abdeca 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -811,9 +811,14 @@ bool xs_watch(struct xs_handle *h, const char
*path, const char *token)
 
 #ifdef USE_PTHREAD
 #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
-#define READ_THREAD_STACKSIZE                                  \
-       ((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ?       \
-       PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
+
+/* NetBSD doesn't have PTHREAD_STACK_MIN. */
+#ifndef PTHREAD_STACK_MIN
+# define PTHREAD_STACK_MIN 0
+#endif
+
+#define READ_THREAD_STACKSIZE \
+       MAX(PTHREAD_STACK_MIN, DEFAULT_THREAD_STACKSIZE)
 
        /* We dynamically create a reader thread on demand. */
        mutex_lock(&h->request_mutex);

which makes things rather clearer IMO.

~Andrew

Re: [PATCH] libs/store: make build without PTHREAD_STACK_MIN

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 06:56:46PM +0000, Andrew Cooper wrote:
> On 12/01/2021 18:12, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> >
> > On NetBSD, PTHREAD_STACK_MIN is not available.
> > Just use DEFAULT_THREAD_STACKSIZE if PTHREAD_STACK_MIN is not available.
> >
> > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > ---
> >  tools/libs/store/xs.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> > index 4ac73ec317..8e646b98d6 100644
> > --- a/tools/libs/store/xs.c
> > +++ b/tools/libs/store/xs.c
> > @@ -811,9 +811,13 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
> >  
> >  #ifdef USE_PTHREAD
> >  #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
> > +#ifndef PTHREAD_STACK_MIN
> > +#define READ_THREAD_STACKSIZE DEFAULT_THREAD_STACKSIZE
> > +#else
> >  #define READ_THREAD_STACKSIZE 					\
> >  	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
> >  	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
> > +#endif
> 
> How about this:
> 
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 4ac73ec317..3fa3abdeca 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -811,9 +811,14 @@ bool xs_watch(struct xs_handle *h, const char
> *path, const char *token)
>  
>  #ifdef USE_PTHREAD
>  #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
> -#define READ_THREAD_STACKSIZE                                  \
> -       ((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ?       \
> -       PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
> +
> +/* NetBSD doesn't have PTHREAD_STACK_MIN. */
> +#ifndef PTHREAD_STACK_MIN
> +# define PTHREAD_STACK_MIN 0
> +#endif
> +
> +#define READ_THREAD_STACKSIZE \
> +       MAX(PTHREAD_STACK_MIN, DEFAULT_THREAD_STACKSIZE)
>  
>         /* We dynamically create a reader thread on demand. */
>         mutex_lock(&h->request_mutex);
> 
> which makes things rather clearer IMO.

fine with me too

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] ocaml/libs/eventchn: drop unneeded evtchn.h

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD xen/sys/evtchn.h is not available any more. Just remove it as it's
not needed.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

---
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
index ba40078d09..f889a7a2e4 100644
--- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
+++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
@@ -22,7 +22,6 @@
 #include <stdint.h>
 #include <sys/ioctl.h>
 #include <xen/xen.h>
-#include <xen/sys/evtchn.h>
 #include <xenevtchn.h>
 
 #define CAML_NAME_SPACE
-- 
2.29.2

Re: [PATCH] ocaml/libs/eventchn: drop unneeded evtchn.h

[Christian Lindig]

Acked-by: Christian Lindig <christian.lindig@citrix.com>

________________________________________
From: Manuel Bouyer <bouyer@antioche.eu.org>
Sent: 12 January 2021 18:12
To: xen-devel@lists.xenproject.org
Cc: Manuel Bouyer; Christian Lindig; David Scott; Ian Jackson; Wei Liu
Subject: [PATCH] ocaml/libs/eventchn: drop unneeded evtchn.h

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD xen/sys/evtchn.h is not available any more. Just remove it as it's
not needed.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

---
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
index ba40078d09..f889a7a2e4 100644
--- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
+++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
@@ -22,7 +22,6 @@
 #include <stdint.h>
 #include <sys/ioctl.h>
 #include <xen/xen.h>
-#include <xen/sys/evtchn.h>
 #include <xenevtchn.h>

 #define CAML_NAME_SPACE
--
2.29.2

Re: [PATCH] ocaml/libs/eventchn: drop unneeded evtchn.h

[Manuel Bouyer]

On Wed, Jan 13, 2021 at 09:22:38AM +0000, Christian Lindig wrote:
> Acked-by: Christian Lindig <christian.lindig@citrix.com>

thanks. What should I do now, submit a new patch with this tag, or just wait
for it to be commited ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH] xenpaging.c: include errno.h

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

writable definition of errno on NetBSD.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/xenpaging/xenpaging.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/xenpaging/xenpaging.c b/tools/xenpaging/xenpaging.c
index 33098046c2..6e5490315d 100644
--- a/tools/xenpaging/xenpaging.c
+++ b/tools/xenpaging/xenpaging.c
@@ -22,6 +22,7 @@
 #include <inttypes.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <errno.h>
 #include <stdarg.h>
 #include <time.h>
 #include <signal.h>
-- 
2.29.2

Re: [PATCH] xenpaging.c: include errno.h

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:40PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> writable definition of errno on NetBSD.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH] xenpmd.c: use dynamic allocation

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

On NetBSD, d_name is larger than 256, so file_name[284] may not be large
enough (and gcc emits a format-truncation error).
Use asprintf() instead of snprintf() on a static on-stack buffer.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/xenpmd/xenpmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
index 12b82cf43e..e432aad856 100644
--- a/tools/xenpmd/xenpmd.c
+++ b/tools/xenpmd/xenpmd.c
@@ -101,7 +101,7 @@ FILE *get_next_battery_file(DIR *battery_dir,
 {
     FILE *file = 0;
     struct dirent *dir_entries;
-    char file_name[284];
+    char *file_name;
     int ret;
     
     do 
@@ -112,16 +112,16 @@ FILE *get_next_battery_file(DIR *battery_dir,
         if ( strlen(dir_entries->d_name) < 4 )
             continue;
         if ( battery_info_type == BIF ) 
-            ret = snprintf(file_name, sizeof(file_name), BATTERY_INFO_FILE_PATH,
+            ret = asprintf(&file_name, BATTERY_INFO_FILE_PATH,
                      dir_entries->d_name);
         else 
-            ret = snprintf(file_name, sizeof(file_name), BATTERY_STATE_FILE_PATH,
+            ret = asprintf(&file_name, BATTERY_STATE_FILE_PATH,
                      dir_entries->d_name);
         /* This should not happen but is needed to pass gcc checks */
         if (ret < 0)
             continue;
-        file_name[sizeof(file_name) - 1] = '\0';
         file = fopen(file_name, "r");
+	free(file_name);
     } while ( !file );
 
     return file;
-- 
2.29.2

Re: [PATCH] xenpmd.c: use dynamic allocation

[Ian Jackson]

Manuel Bouyer writes ("[PATCH] xenpmd.c: use dynamic allocation"):
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> On NetBSD, d_name is larger than 256, so file_name[284] may not be large
> enough (and gcc emits a format-truncation error).
> Use asprintf() instead of snprintf() on a static on-stack buffer.

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH] xenstat_netbsd: remove usused code

[Manuel Bouyer]

From: Manuel Bouyer <bouyer@netbsd.org>

remove PROCNETDEV_HEADER[] and read_attributes_vbd(), gcc complains that they
are unused

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/stat/xenstat_netbsd.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/tools/libs/stat/xenstat_netbsd.c b/tools/libs/stat/xenstat_netbsd.c
index 6e9d6aee10..64eda9e1ae 100644
--- a/tools/libs/stat/xenstat_netbsd.c
+++ b/tools/libs/stat/xenstat_netbsd.c
@@ -55,11 +55,6 @@ get_priv_data(xenstat_handle *handle)
 }
 
 /* Expected format of /proc/net/dev */
-static const char PROCNETDEV_HEADER[] =
-    "Inter-|   Receive                                                |"
-    "  Transmit\n"
-    " face |bytes    packets errs drop fifo frame compressed multicast|"
-    "bytes    packets errs drop fifo colls carrier compressed\n";
 
 /* Collect information about networks */
 int xenstat_collect_networks(xenstat_node * node)
@@ -76,12 +71,6 @@ void xenstat_uninit_networks(xenstat_handle * handle)
 		fclose(priv->procnetdev);
 }
 
-static int read_attributes_vbd(const char *vbd_directory, const char *what, char *ret, int cap)
-{
-	/* XXX implement */
-	return 0;
-}
-
 /* Collect information about VBDs */
 int xenstat_collect_vbds(xenstat_node * node)
 {
-- 
2.29.2

Re: [PATCH] xenstat_netbsd: remove usused code

[Roger Pau Monné]

On Tue, Jan 12, 2021 at 07:12:42PM +0100, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> remove PROCNETDEV_HEADER[] and read_attributes_vbd(), gcc complains that they
> are unused
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] Fix error: array subscript has type 'char'

[Jan Beulich]

On 12.01.2021 19:12, Manuel Bouyer wrote:
> From: Manuel Bouyer <bouyer@netbsd.org>
> 
> Use unsigned char variable, or cast to (unsigned char), for
> tolower()/islower() and friends. Fix compiler error
> array subscript has type 'char' [-Werror=char-subscripts]

Isn't this something that wants changing in your ctype.h instead?
the functions (or macros), as per the C standard, ought to accept
plain char aiui, and if they use the input as an array subscript,
it should be their implementation suitably converting type first.

Jan

Re: [PATCH] Fix error: array subscript has type 'char'

[Manuel Bouyer]

On Thu, Jan 14, 2021 at 11:53:20AM +0100, Jan Beulich wrote:
> On 12.01.2021 19:12, Manuel Bouyer wrote:
> > From: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > Use unsigned char variable, or cast to (unsigned char), for
> > tolower()/islower() and friends. Fix compiler error
> > array subscript has type 'char' [-Werror=char-subscripts]
> 
> Isn't this something that wants changing in your ctype.h instead?
> the functions (or macros), as per the C standard, ought to accept
> plain char aiui, and if they use the input as an array subscript,
> it should be their implementation suitably converting type first.

I asked for inputs from NetBSD developers familiar with this part.

Although the parameter is an int, only a subset of values is valid,
as stated in ISO C 2018 (Section 7.4 paragrah 1):
> In all cases the argument is an int, the value of which shall be
> representable as an unsigned char or shall equal the value of the
> macro EOF.  If the argument has any other value, the behavior is 
> undefined.                               


As stated by NetBSD's ctype(3) manual page, NetBSD and glibc took different
approach. NetBSD emits a compile-time warning if the input may lead to
undefined behavior. quoting the man page:
     Some implementations of libc, such as glibc as of 2018, attempt to avoid
     the worst of the undefined behavior by defining the functions to work for
     all integer inputs representable by either unsigned char or char, and
     suppress the warning.  However, this is not an excuse for avoiding
     conversion to unsigned char: if EOF coincides with any such value, as it
     does when it is -1 on platforms with signed char, programs that pass char
     will still necessarily confuse the classification and mapping of EOF with
     the classification and mapping of some non-EOF inputs.


So, although no warning is emmited on linux, it looks like to me that the
cast to unsigned char is needed anyway, and relying on glibc's behavior
is not portable.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] Fix error: array subscript has type 'char'

[Jan Beulich]

On 14.01.2021 13:29, Manuel Bouyer wrote:
> On Thu, Jan 14, 2021 at 11:53:20AM +0100, Jan Beulich wrote:
>> On 12.01.2021 19:12, Manuel Bouyer wrote:
>>> From: Manuel Bouyer <bouyer@netbsd.org>
>>>
>>> Use unsigned char variable, or cast to (unsigned char), for
>>> tolower()/islower() and friends. Fix compiler error
>>> array subscript has type 'char' [-Werror=char-subscripts]
>>
>> Isn't this something that wants changing in your ctype.h instead?
>> the functions (or macros), as per the C standard, ought to accept
>> plain char aiui, and if they use the input as an array subscript,
>> it should be their implementation suitably converting type first.
> 
> I asked for inputs from NetBSD developers familiar with this part.
> 
> Although the parameter is an int, only a subset of values is valid,
> as stated in ISO C 2018 (Section 7.4 paragrah 1):
>> In all cases the argument is an int, the value of which shall be
>> representable as an unsigned char or shall equal the value of the
>> macro EOF.  If the argument has any other value, the behavior is 
>> undefined.                               

Which means you're shifting the undefined-ness from the implementation
(using the value as array index) to the callers (truncating values, or
converting value range). In particular I do not think that the
intention behind the standard's wording is for every caller to need to
cast to unsigned char, when e.g. character literals are of type char
and string literals are of type const char[].

> As stated by NetBSD's ctype(3) manual page, NetBSD and glibc took different
> approach. NetBSD emits a compile-time warning if the input may lead to
> undefined behavior. quoting the man page:
>      Some implementations of libc, such as glibc as of 2018, attempt to avoid
>      the worst of the undefined behavior by defining the functions to work for
>      all integer inputs representable by either unsigned char or char, and
>      suppress the warning.  However, this is not an excuse for avoiding
>      conversion to unsigned char: if EOF coincides with any such value, as it
>      does when it is -1 on platforms with signed char, programs that pass char
>      will still necessarily confuse the classification and mapping of EOF with
>      the classification and mapping of some non-EOF inputs.
> 
> 
> So, although no warning is emmited on linux, it looks like to me that the
> cast to unsigned char is needed anyway, and relying on glibc's behavior
> is not portable.

I fully agree we shouldn't rely on glibc's behavior (I'm sure
you've looked at xen/include/xen/ctype.h to see how we address
this it Xen itself, but I will admit this is to a degree comparing
apples and oranges, not the least because the lack of a need to
consider EOF in Xen). At least in xen/tools/symbols.c I don't
think we're at risk of running into "undefined" space. Casts are
generally desirable to avoid whenever possible, as they come with
their own set of risks. Granted this is "only" user space code,
but still.

Jan

Re: [PATCH] Fix error: array subscript has type 'char'

[Manuel Bouyer]

On Thu, Jan 14, 2021 at 02:25:05PM +0100, Jan Beulich wrote:
> On 14.01.2021 13:29, Manuel Bouyer wrote:
> > On Thu, Jan 14, 2021 at 11:53:20AM +0100, Jan Beulich wrote:
> >> On 12.01.2021 19:12, Manuel Bouyer wrote:
> >>> From: Manuel Bouyer <bouyer@netbsd.org>
> >>>
> >>> Use unsigned char variable, or cast to (unsigned char), for
> >>> tolower()/islower() and friends. Fix compiler error
> >>> array subscript has type 'char' [-Werror=char-subscripts]
> >>
> >> Isn't this something that wants changing in your ctype.h instead?
> >> the functions (or macros), as per the C standard, ought to accept
> >> plain char aiui, and if they use the input as an array subscript,
> >> it should be their implementation suitably converting type first.
> > 
> > I asked for inputs from NetBSD developers familiar with this part.
> > 
> > Although the parameter is an int, only a subset of values is valid,
> > as stated in ISO C 2018 (Section 7.4 paragrah 1):
> >> In all cases the argument is an int, the value of which shall be
> >> representable as an unsigned char or shall equal the value of the
> >> macro EOF.  If the argument has any other value, the behavior is 
> >> undefined.                               
> 
> Which means you're shifting the undefined-ness from the implementation
> (using the value as array index) to the callers (truncating values, or
> converting value range). In particular I do not think that the
> intention behind the standard's wording is for every caller to need to
> cast to unsigned char, when e.g. character literals are of type char
> and string literals are of type const char[].

If you don't cast you fall into the undefined behavior case for non-ascii
characters. For example, "é" in iso-latin-1 is 0xe9. In a signed char, this is
-23 (decimal). without the cast, tolower() will see -23.
If it is casted to (unsigned char) first, tolower() will see 233, as expected.
The following test program illustrates this:
armandeche:/tmp>cat toto.c
#include <stdio.h>

int
main(int _c, const char *_v[])
{
        char c = 0xe9;
	printf("%d %d\n", (int)c, (int)(unsigned char)c);
}
armandeche:/tmp>./toto 
-23 233



> 
> > As stated by NetBSD's ctype(3) manual page, NetBSD and glibc took different
> > approach. NetBSD emits a compile-time warning if the input may lead to
> > undefined behavior. quoting the man page:
> >      Some implementations of libc, such as glibc as of 2018, attempt to avoid
> >      the worst of the undefined behavior by defining the functions to work for
> >      all integer inputs representable by either unsigned char or char, and
> >      suppress the warning.  However, this is not an excuse for avoiding
> >      conversion to unsigned char: if EOF coincides with any such value, as it
> >      does when it is -1 on platforms with signed char, programs that pass char
> >      will still necessarily confuse the classification and mapping of EOF with
> >      the classification and mapping of some non-EOF inputs.
> > 
> > 
> > So, although no warning is emmited on linux, it looks like to me that the
> > cast to unsigned char is needed anyway, and relying on glibc's behavior
> > is not portable.
> 
> I fully agree we shouldn't rely on glibc's behavior (I'm sure
> you've looked at xen/include/xen/ctype.h to see how we address
> this it Xen itself, but I will admit this is to a degree comparing
> apples and oranges, not the least because the lack of a need to
> consider EOF in Xen). At least in xen/tools/symbols.c I don't
> think we're at risk of running into "undefined" space. Casts are

as long as there's only ascii characters.

Anyway NetBSD won't change its ctype.h

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] Fix error: array subscript has type 'char'

[Manuel Bouyer]

On Thu, Jan 14, 2021 at 03:16:15PM +0100, Manuel Bouyer wrote:
> On Thu, Jan 14, 2021 at 02:25:05PM +0100, Jan Beulich wrote:
> > On 14.01.2021 13:29, Manuel Bouyer wrote:
> > > On Thu, Jan 14, 2021 at 11:53:20AM +0100, Jan Beulich wrote:
> > >> On 12.01.2021 19:12, Manuel Bouyer wrote:
> > >>> From: Manuel Bouyer <bouyer@netbsd.org>
> > >>>
> > >>> Use unsigned char variable, or cast to (unsigned char), for
> > >>> tolower()/islower() and friends. Fix compiler error
> > >>> array subscript has type 'char' [-Werror=char-subscripts]
> > >>
> > >> Isn't this something that wants changing in your ctype.h instead?
> > >> the functions (or macros), as per the C standard, ought to accept
> > >> plain char aiui, and if they use the input as an array subscript,
> > >> it should be their implementation suitably converting type first.
> > > 
> > > I asked for inputs from NetBSD developers familiar with this part.
> > > 
> > > Although the parameter is an int, only a subset of values is valid,
> > > as stated in ISO C 2018 (Section 7.4 paragrah 1):
> > >> In all cases the argument is an int, the value of which shall be
> > >> representable as an unsigned char or shall equal the value of the
> > >> macro EOF.  If the argument has any other value, the behavior is 
> > >> undefined.                               
> > 
> > Which means you're shifting the undefined-ness from the implementation
> > (using the value as array index) to the callers (truncating values, or
> > converting value range). In particular I do not think that the
> > intention behind the standard's wording is for every caller to need to
> > cast to unsigned char, when e.g. character literals are of type char
> > and string literals are of type const char[].
> 
> If you don't cast you fall into the undefined behavior case for non-ascii
> characters. For example, "é" in iso-latin-1 is 0xe9. In a signed char, this is
> -23 (decimal). without the cast, tolower() will see -23.
> If it is casted to (unsigned char) first, tolower() will see 233, as expected.
> The following test program illustrates this:
> armandeche:/tmp>cat toto.c
> #include <stdio.h>
> 
> int
> main(int _c, const char *_v[])
> {
>         char c = 0xe9;
> 	printf("%d %d\n", (int)c, (int)(unsigned char)c);
> }
> armandeche:/tmp>./toto 
> -23 233
> 
> 
> 
> > 
> > > As stated by NetBSD's ctype(3) manual page, NetBSD and glibc took different
> > > approach. NetBSD emits a compile-time warning if the input may lead to
> > > undefined behavior. quoting the man page:
> > >      Some implementations of libc, such as glibc as of 2018, attempt to avoid
> > >      the worst of the undefined behavior by defining the functions to work for
> > >      all integer inputs representable by either unsigned char or char, and
> > >      suppress the warning.  However, this is not an excuse for avoiding
> > >      conversion to unsigned char: if EOF coincides with any such value, as it
> > >      does when it is -1 on platforms with signed char, programs that pass char
> > >      will still necessarily confuse the classification and mapping of EOF with
> > >      the classification and mapping of some non-EOF inputs.
> > > 
> > > 
> > > So, although no warning is emmited on linux, it looks like to me that the
> > > cast to unsigned char is needed anyway, and relying on glibc's behavior
> > > is not portable.
> > 
> > I fully agree we shouldn't rely on glibc's behavior (I'm sure
> > you've looked at xen/include/xen/ctype.h to see how we address
> > this it Xen itself, but I will admit this is to a degree comparing
> > apples and oranges, not the least because the lack of a need to
> > consider EOF in Xen). At least in xen/tools/symbols.c I don't
> > think we're at risk of running into "undefined" space. Casts are
> 
> as long as there's only ascii characters.
> 
> Anyway NetBSD won't change its ctype.h

I guess I'm going to give up on this one. We'll keep it as a local patch.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH] Fix error: array subscript has type 'char'

[Ian Jackson]

Manuel Bouyer writes ("Re: [PATCH] Fix error: array subscript has type 'char'"):
> On Thu, Jan 14, 2021 at 03:16:15PM +0100, Manuel Bouyer wrote:
> > On Thu, Jan 14, 2021 at 02:25:05PM +0100, Jan Beulich wrote:
> > > Which means you're shifting the undefined-ness from the implementation

The undefined-ness is in the *specification*. [1]

> > > Isn't this something that wants changing in your ctype.h instead?
> > > the functions (or macros), as per the C standard

Jan, can you please check your facts before asserting the existence
of a pretty blatant bug in a platform's implementation of basic C
functions ?

From my copy of C99+TC1+TC2, para 7.4:

 1   In all cases the argument is an int, the value of which shall be
     representable as an unsigned char or shall equal the value of the
     macro EOF.  If the argument has any other value, the behavior is
     undefined. [...]

If char is signed, then it can contain -ve values.  Those values are
promoted to int by the usual integer promotions.  Naturally such
negative values are not representable as unsigned char.  Passing them
to ctype macros is UB.

So Manuel's ctypes.h is conforming and the compiler warning (which I
have seen on Linux too) is correct.

> I guess I'm going to give up on this one. We'll keep it as a local patch.

Manuel, your original patch:

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Release-Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(The R-A is FTAOD since IMO this is a bugfix.)

Ian.

[1] I agree that "fixing" ctypes.h everywhere might be nice (eg the
way glibc does it) but that has other implications and it is not
reasonable to expect platforms supporting Xen to do that.

Re: [PATCH] Fix error: array subscript has type 'char'

[Jan Beulich]

On 26.01.2021 18:59, Ian Jackson wrote:
> Manuel Bouyer writes ("Re: [PATCH] Fix error: array subscript has type 'char'"):
>> On Thu, Jan 14, 2021 at 03:16:15PM +0100, Manuel Bouyer wrote:
>>> On Thu, Jan 14, 2021 at 02:25:05PM +0100, Jan Beulich wrote:
>>>> Which means you're shifting the undefined-ness from the implementation
> 
> The undefined-ness is in the *specification*. [1]
> 
>>>> Isn't this something that wants changing in your ctype.h instead?
>>>> the functions (or macros), as per the C standard
> 
> Jan, can you please check your facts before asserting the existence
> of a pretty blatant bug in a platform's implementation of basic C
> functions ?
> 
> From my copy of C99+TC1+TC2, para 7.4:
> 
>  1   In all cases the argument is an int, the value of which shall be
>      representable as an unsigned char or shall equal the value of the
>      macro EOF.  If the argument has any other value, the behavior is
>      undefined. [...]
> 
> If char is signed, then it can contain -ve values.  Those values are
> promoted to int by the usual integer promotions.  Naturally such
> negative values are not representable as unsigned char.  Passing them
> to ctype macros is UB.

I did read that part of the spec before replying. Irrespective
of the wording there it seems entirely unreasonable to me for
the spec to imply all use sites of the is...() functions to
have to use casts. Even more so that we all know (I suppose)
that casts can be dangerous as both potentially introducing
bugs (perhaps not at the point of their addition, but later
when code elsewhere gets changed) and keeping analysis tools
from actually spotting ones.

But yes, I'm not the maintainer of this code, so if you're
happy with such risks, so be it.

For the record, to me the less risky approach here would seem
to have been to make use of compilers allowing to choose
whether plain char is signed or unsigned, and force it to
unsigned for at least the affected components.

Jan

Re: [PATCH] Fix error: array subscript has type 'char'

[Jan Beulich]

On 27.01.2021 09:31, Jan Beulich wrote:
> But yes, I'm not the maintainer of this code, so if you're
> happy with such risks, so be it.

And actually I was only partly right here - there's one hunk
here affecting code I'm the maintainer just as much as you
are. At least there I'd like to ask that ...

> For the record, to me the less risky approach here would seem
> to have been to make use of compilers allowing to choose
> whether plain char is signed or unsigned, and force it to
> unsigned for at least the affected components.

... this be considered as an alternative, before I maybe
withdraw my de-facto nak.

(To amend my earlier reply: Such a command line option
addition could then also be properly commented, to explain
the particular need for the option.)

Jan

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char'"):
> I did read that part of the spec before replying.

I find this quite astonishing.  You claimed that FreeBSD's libc was
buggy *after having read the spec to which you agree it conforms*.

>   Irrespective of the wording there it seems entirely unreasonable
> to me for the spec to imply all use sites of the is...() functions
> to have to use casts. Even more so that we all know (I suppose) that
> casts can be dangerous as both potentially introducing bugs (perhaps
> not at the point of their addition, but later when code elsewhere
> gets changed) and keeping analysis tools from actually spotting
> ones.

Nevertheless, this is the design of the C standard.  A common approach
to this problem is something like this (from libxl_internal.h):

  /*
   * int CTYPE(ISFOO, char c);
   * int CTYPE(toupper, char c);
   * int CTYPE(tolower, char c);
   *
   * This is necessary because passing a simple char to a ctype.h
   * is forbidden.  ctype.h macros take ints derived from _unsigned_ chars.
   *
   * If you have a char which might be EOF then you should already have
   * it in an int representing an unsigned char, and you can use the
   * <ctype.h> macros directly.  This generally happens only with values
   * from fgetc et al.
   *
   * For any value known to be a character (eg, anything that came from
   * a char[]), use CTYPE.
   */
  #define CTYPE(isfoo,c) (isfoo((unsigned char)(c)))

Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char'"):
> On 27.01.2021 09:31, Jan Beulich wrote:
> > But yes, I'm not the maintainer of this code, so if you're
> > happy with such risks, so be it.
> 
> And actually I was only partly right here - there's one hunk
> here affecting code I'm the maintainer just as much as you
> are. At least there I'd like to ask that ...
> 
> > For the record, to me the less risky approach here would seem
> > to have been to make use of compilers allowing to choose
> > whether plain char is signed or unsigned, and force it to
> > unsigned for at least the affected components.
> 
> ... this be considered as an alternative, before I maybe
> withdraw my de-facto nak.

Whether char is signed or unsigned is generally specified in the
platform API/ABI.  Deviating from this for userland code is not
possible or reasonable since it would involve processing the system
headers with a deviant langauge definition!

Deviating from it for hypervisor code would be possible but I think it
would be unwise.

Ian.

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[Jan Beulich]

On 27.01.2021 14:53, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char'"):
>> On 27.01.2021 09:31, Jan Beulich wrote:
>>> But yes, I'm not the maintainer of this code, so if you're
>>> happy with such risks, so be it.
>>
>> And actually I was only partly right here - there's one hunk
>> here affecting code I'm the maintainer just as much as you
>> are. At least there I'd like to ask that ...
>>
>>> For the record, to me the less risky approach here would seem
>>> to have been to make use of compilers allowing to choose
>>> whether plain char is signed or unsigned, and force it to
>>> unsigned for at least the affected components.
>>
>> ... this be considered as an alternative, before I maybe
>> withdraw my de-facto nak.
> 
> Whether char is signed or unsigned is generally specified in the
> platform API/ABI.  Deviating from this for userland code is not
> possible or reasonable since it would involve processing the system
> headers with a deviant langauge definition!

I don't think I've ever come across that part of a platform
API/ABI spec. Instead my understanding so far was that good
platform headers would be ignorant of the user's choice of
char's signed-ness (wherever compilers offer such a choice,
but I think all that I've ever worked with did). At the very
least gcc's doc doesn't warn about any possible
incompatibilities resulting from use of -fsigned-char or
-funsigned-char (or their bitfield equivalents, for that
matter).

Jan

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]"):
> I don't think I've ever come across that part of a platform
> API/ABI spec. Instead my understanding so far was that good
> platform headers would be ignorant of the user's choice of
> char's signed-ness (wherever compilers offer such a choice,
> but I think all that I've ever worked with did). At the very
> least gcc's doc doesn't warn about any possible
> incompatibilities resulting from use of -fsigned-char or
> -funsigned-char (or their bitfield equivalents, for that
> matter).

Well, I've considered this and I still don't think changing to
-funsigned-char is good idea.

Are you OK with me checking in the current patch or should I ask the
other committers for a second opinion ?

Thanks,
Ian.

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[Jan Beulich]

On 27.01.2021 17:21, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]"):
>> I don't think I've ever come across that part of a platform
>> API/ABI spec. Instead my understanding so far was that good
>> platform headers would be ignorant of the user's choice of
>> char's signed-ness (wherever compilers offer such a choice,
>> but I think all that I've ever worked with did). At the very
>> least gcc's doc doesn't warn about any possible
>> incompatibilities resulting from use of -fsigned-char or
>> -funsigned-char (or their bitfield equivalents, for that
>> matter).
> 
> Well, I've considered this and I still don't think changing to
> -funsigned-char is good idea.
> 
> Are you OK with me checking in the current patch or should I ask the
> other committers for a second opinion ?

For the changes to tools/ it's really up to you. For the change
to xen/tools/symbols.c I could live with it (for being user
space code), but I still think adding casts in such a place is
not necessarily setting a good precedent. So for this one I'd
indeed appreciate getting another opinion.

Jan

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[George Dunlap]

> On Jan 27, 2021, at 4:32 PM, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 27.01.2021 17:21, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]"):
>>> I don't think I've ever come across that part of a platform
>>> API/ABI spec. Instead my understanding so far was that good
>>> platform headers would be ignorant of the user's choice of
>>> char's signed-ness (wherever compilers offer such a choice,
>>> but I think all that I've ever worked with did). At the very
>>> least gcc's doc doesn't warn about any possible
>>> incompatibilities resulting from use of -fsigned-char or
>>> -funsigned-char (or their bitfield equivalents, for that
>>> matter).
>> 
>> Well, I've considered this and I still don't think changing to
>> -funsigned-char is good idea.
>> 
>> Are you OK with me checking in the current patch or should I ask the
>> other committers for a second opinion ?
> 
> For the changes to tools/ it's really up to you. For the change
> to xen/tools/symbols.c I could live with it (for being user
> space code), but I still think adding casts in such a place is
> not necessarily setting a good precedent. So for this one I'd
> indeed appreciate getting another opinion.

My thoughts:

* On the whole, the risk of an incompatibility with system headers does seem higher than the risk of casting a value which is known not to be EOF

* Such a change doesn’t seem like the kind of thing we should ask Manuel to do, when a simpler change will do the trick

* At any rate it doesn’t seem like a good thing to experiment with in the week before the feature freeze.

 -George

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[Jan Beulich]

On 27.01.2021 17:52, George Dunlap wrote:
> 
> 
>> On Jan 27, 2021, at 4:32 PM, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 27.01.2021 17:21, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]"):
>>>> I don't think I've ever come across that part of a platform
>>>> API/ABI spec. Instead my understanding so far was that good
>>>> platform headers would be ignorant of the user's choice of
>>>> char's signed-ness (wherever compilers offer such a choice,
>>>> but I think all that I've ever worked with did). At the very
>>>> least gcc's doc doesn't warn about any possible
>>>> incompatibilities resulting from use of -fsigned-char or
>>>> -funsigned-char (or their bitfield equivalents, for that
>>>> matter).
>>>
>>> Well, I've considered this and I still don't think changing to
>>> -funsigned-char is good idea.
>>>
>>> Are you OK with me checking in the current patch or should I ask the
>>> other committers for a second opinion ?
>>
>> For the changes to tools/ it's really up to you. For the change
>> to xen/tools/symbols.c I could live with it (for being user
>> space code), but I still think adding casts in such a place is
>> not necessarily setting a good precedent. So for this one I'd
>> indeed appreciate getting another opinion.
> 
> My thoughts:
> 
> * On the whole, the risk of an incompatibility with system headers does seem higher than the risk of casting a value which is known not to be EOF
> 
> * Such a change doesn’t seem like the kind of thing we should ask Manuel to do, when a simpler change will do the trick
> 
> * At any rate it doesn’t seem like a good thing to experiment with in the week before the feature freeze.

Well, okay then, I withdraw my implied nak under these conditions.

Jan

Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]

[Ian Jackson]

George Dunlap writes ("Re: [PATCH] Fix error: array subscript has type 'char' [and 1 more messages]"):
> > On Jan 27, 2021, at 4:32 PM, Jan Beulich <jbeulich@suse.com> wrote:
> > On 27.01.2021 17:21, Ian Jackson wrote:
> >> Are you OK with me checking in the current patch or should I ask the
> >> other committers for a second opinion ?
> > 
> > For the changes to tools/ it's really up to you. For the change
> > to xen/tools/symbols.c I could live with it (for being user
> > space code), but I still think adding casts in such a place is
> > not necessarily setting a good precedent. So for this one I'd
> > indeed appreciate getting another opinion.
> 
> My thoughts:
> 
> * On the whole, the risk of an incompatibility with system headers does seem higher than the risk of casting a value which is known not to be EOF
> 
> * Such a change doesn’t seem like the kind of thing we should ask Manuel to do, when a simpler change will do the trick
> 
> * At any rate it doesn’t seem like a good thing to experiment with in the week before the feature freeze.

Thanks, George.  Given the release timing, I intend to commit Manuel's
patch unless I hear a further contrary opinion before 1700 UTC
tomorrow.

NB that with my RM hat on I consider this a bugfix so it would not
need a freeze exception to be committed next week but obviously with
my RM hat on I would prefer no to delay it.

Ian.

Re: NetBSD patches

[Andrew Cooper]

Hello,

I've committed some of the trivial and reviewed patches.

I think all others have outstanding questions, or really need the
identified maintainers to comment.

~Andrew

Re: NetBSD patches

[Manuel Bouyer]

On Mon, Jan 18, 2021 at 07:08:58PM +0000, Andrew Cooper wrote:
> Hello,
> 
> I've committed some of the trivial and reviewed patches.
> 
> I think all others have outstanding questions, or really need the
> identified maintainers to comment.

thanks. I will try to adress the questions later this week.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--