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

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.

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

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.

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