Re: [PATCH] tools/xendomains: Only save/restore/migrate if supported by xenlight

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote:
> From: Peter Hoyes <Peter.Hoyes@xxxxxxx>
> 
> Saving, restoring and migrating domains are not currently supported on
> arm and arm64 platforms, so xendomains prints the warning:
> 
>   An error occurred while saving domain:
>   command not implemented
> 
> when attempting to run `xendomains stop`. It otherwise continues to shut
> down the domains cleanly, with the unsupported steps skipped.

The patch looks kind of ok, but shouldn't $XENDOMAINS_SAVE be set to an
empty string in the config by the admin instead?

Or is the issue that $XENDOMAINS_SAVE is set by default, even on arm* ?

Maybe it's easier to check that the command is implemented at run time
rather than trying to have a good default value for XENDOMAINS_SAVE at
install/package time.

> Use `xl help` to detect whether save/restore/migrate is supported by the
> platform. If not, do not attempt to run the corresponding command.
> 
> Signed-off-by: Peter Hoyes <Peter.Hoyes@xxxxxxx>
> ---
>  tools/hotplug/Linux/xendomains.in | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/xendomains.in 
> b/tools/hotplug/Linux/xendomains.in
> index 70f4129ef4..bafcb874e1 100644
> --- a/tools/hotplug/Linux/xendomains.in
> +++ b/tools/hotplug/Linux/xendomains.in
> @@ -229,6 +229,15 @@ parseln()
>      [ -n "$name" -a -n "$id" ] && return 0 || return 1
>  }
>  
> +subcmd_supported()
> +{
> +    local output
> +    output=$("$CMD help | grep "^ $1"")
> +    if [ ! "$output" ]; then
> +        return 1
> +    fi

It looks like some quote are in the wrong place. You probably wanted to
write:
    output="$($CMD help | grep "^ $1")"

But I'd like to have this slightly more robust, to match the whole
command, rather than the beginning.
(For example `subcmd_supported "pci"` would reture true even if no "pci"
command exist.)

Something like:
    $CMD help | grep "^ $1\( \|$\)"
To check that the command name is followed by a space or end-of-line
(even if it seems that there's always a space printed.)
A similar pattern is used in "tools/xl/bash-completion", so it should
probably be fine in the future.

Then, we don't really need the "$output" from grep, we could just ask it
if there's a match or not, with --quiet:
    $CMD help | grep -q "^ $1\( \|$\)"

And that would be the whole function. Would that works for you?


The rest of the patch looks fine.

Thanks,

-- 
Anthony PERARD