| Message ID | 20230322135800.3869458-1-peter.hoyes@arm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tools/xendomains: Only save/restore/migrate if supported by xenlight | expand |
On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote: > From: Peter Hoyes <Peter.Hoyes@arm.com> > > 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@arm.com> > --- > 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,
On 04/04/2023 17:28, Anthony PERARD wrote: > On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote: >> From: Peter Hoyes <Peter.Hoyes@arm.com> >> >> 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* ? Yea the default is the issue. We are building for embedded, using Yocto, so there isn't really an admin. > > 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. It would be cleaner to do this at build time for sure, but I'm not sure the autotools config file approach for sysconfig.xendomains.in can handle the logic for this? Thanks, Peter
On Thu, Apr 06, 2023 at 03:57:12PM +0100, Peter Hoyes wrote: > On 04/04/2023 17:28, Anthony PERARD wrote: > > On Wed, Mar 22, 2023 at 01:58:00PM +0000, Peter Hoyes wrote: > > > From: Peter Hoyes <Peter.Hoyes@arm.com> > > > > > > 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* ? > Yea the default is the issue. We are building for embedded, using Yocto, so > there isn't really an admin. > > > > 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. > > It would be cleaner to do this at build time for sure, but I'm not sure the > autotools config file approach for sysconfig.xendomains.in can handle the > logic for this? I think that possible, we can already change config in ./configure based on the CPU. I'm preparing a patch. Thanks,
> On 22 Mar 2023, at 13:58, Peter Hoyes <Peter.Hoyes@arm.com> wrote: > > From: Peter Hoyes <Peter.Hoyes@arm.com> > > 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. > > 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@arm.com> > --- Hi Anthony, Regarding this patch, is there any update? I’m asking just because here https://patchwork.kernel.org/project/xen-devel/patch/20230322135800.3869458-1-peter.hoyes@arm.com/ it seems you were volunteering to fix that in another way. Cheers, Luca
On Thu, Oct 19, 2023 at 12:50:52PM +0000, Luca Fancellu wrote: > > > > On 22 Mar 2023, at 13:58, Peter Hoyes <Peter.Hoyes@arm.com> wrote: > > > > From: Peter Hoyes <Peter.Hoyes@arm.com> > > > > 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. > > > > 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@arm.com> > > --- > > Hi Anthony, > > Regarding this patch, is there any update? > > I’m asking just because here https://patchwork.kernel.org/project/xen-devel/patch/20230322135800.3869458-1-peter.hoyes@arm.com/ it seems you were volunteering to fix that in another way. I did, https://patchwork.kernel.org/project/xen-devel/patch/20230519162454.50337-1-anthony.perard@citrix.com/ But looks like I need to update the patch. I think it's a bit late for 3.18, but I should be able to update the patch for next release. Cheers,
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 +} + is_running() { get_xsdomid @@ -260,7 +269,8 @@ start() saved_domains=" " if [ "$XENDOMAINS_RESTORE" = "true" ] && - contains_something "$XENDOMAINS_SAVE" + contains_something "$XENDOMAINS_SAVE" && + subcmd_supported "restore" then echo -n "Restoring Xen domains:" saved_domains=`ls $XENDOMAINS_SAVE` @@ -411,7 +421,7 @@ stop() echo -n "(zomb)" continue fi - if test -n "$XENDOMAINS_MIGRATE"; then + if test -n "$XENDOMAINS_MIGRATE" && subcmd_supported "migrate"; then echo -n "(migr)" watchdog_xencmd migrate & WDOG_PID=$! @@ -430,7 +440,7 @@ stop() continue fi fi - if test -n "$XENDOMAINS_SAVE"; then + if test -n "$XENDOMAINS_SAVE" && subcmd_supported "save"; then echo -n "(save)" watchdog_xencmd save & WDOG_PID=$!