Re: [Xen-devel] [PATCH v3] kexec: implemented XEN KEXEC STATUS to determine if an image is loaded

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Tue, Jan 24, 2017 at 12:55:35PM -0600, Eric DeVolder wrote:
> Instead of the scripts having to poke at various fields we can
> provide that functionality via the -S parameter.
>
> kexec_loaded/kexec_crash_loaded exposes Linux kernel kexec/crash
> state. It does not say anything about Xen kexec/crash state. So,
> we need a special approach to get the latter. Though for
> compatibility we provide similar functionality in kexec-tools
> for the former.
>
> This change enables the --status or -S option to work either
> with or without Xen.
>
> Returns 0 if the payload is loaded. Can be used in combination
> with -l or -p to get the state of the proper kexec image.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Signed-off-by: Eric DeVolder <eric.devolder@xxxxxxxxxx>
> ---
> Note: The corresponding Xen changes have been committed
> to the Xen staging branch. Follow this thread:
> https://lists.xenproject.org/archives/html/xen-devel/2017-01/msg01570.html
>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: kexec@xxxxxxxxxxxxxxxxxxx
> CC: xen-devel@xxxxxxxxxxxxxxxxxxxx
> CC: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>
> v0: First version (internal product).
> v1: Posted on kexec mailing list. Changed -s to -S
> v2: Incorporated feedback from kexec mailing list, posted on kexec mailing 
> list
> v3: Incorporated feedback from kexec mailing list
> ---
>  configure.ac      |  8 ++++++-
>  kexec/kexec-xen.c | 26 +++++++++++++++++++++++
>  kexec/kexec.8     |  6 ++++++
>  kexec/kexec.c     | 62 
> ++++++++++++++++++++++++++++++++++++++++++++++++-------
>  kexec/kexec.h     |  5 ++++-
>  5 files changed, 98 insertions(+), 9 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 3044185..c6e864b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -165,8 +165,14 @@ fi
>  dnl find Xen control stack libraries
>  if test "$with_xen" = yes ; then
>       AC_CHECK_HEADER(xenctrl.h,
> -             [AC_CHECK_LIB(xenctrl, xc_kexec_load, ,
> +             [AC_CHECK_LIB(xenctrl, xc_kexec_load, [ have_xenctrl_h=yes ],
>               AC_MSG_NOTICE([Xen support disabled]))])
> +if test "$have_xenctrl_h" = yes ; then
> +     AC_CHECK_LIB(xenctrl, xc_kexec_status,
> +             AC_DEFINE(HAVE_KEXEC_CMD_STATUS, 1,
> +                     [The kexec_status call is available]),
> +             AC_MSG_NOTICE([The kexec_status call is not available]))
> +fi

I have a feeling that you have missed my comment. Please add two TABs
starting from "+if test "$have_xenctrl_h" = yes ; then" up to "+fi".
So, it should look more or less like this:

                AC_MSG_NOTICE([Xen support disabled]))])
+               if test "$have_xenctrl_h" = yes ; then
+                       AC_CHECK_LIB(xenctrl, xc_kexec_status,
...

If it is not needed or something like that please drop me a line.

>  fi
>
>  dnl ---Sanity checks
> diff --git a/kexec/kexec-xen.c b/kexec/kexec-xen.c
> index 24a4191..2b448d3 100644
> --- a/kexec/kexec-xen.c
> +++ b/kexec/kexec-xen.c
> @@ -105,6 +105,27 @@ int xen_kexec_unload(uint64_t kexec_flags)
>       return ret;
>  }
>
> +int xen_kexec_status(uint64_t kexec_flags)
> +{
> +     xc_interface *xch;
> +     uint8_t type;
> +     int ret = -1;
> +
> +#ifdef HAVE_KEXEC_CMD_STATUS
> +     xch = xc_interface_open(NULL, NULL, 0);
> +     if (!xch)
> +             return -1;
> +
> +     type = (kexec_flags & KEXEC_ON_CRASH) ? KEXEC_TYPE_CRASH : 
> KEXEC_TYPE_DEFAULT;
> +
> +     ret = xc_kexec_status(xch, type);
> +
> +     xc_interface_close(xch);
> +#endif
> +
> +     return ret;
> +}
> +
>  void xen_kexec_exec(void)
>  {
>       xc_interface *xch;
> @@ -130,6 +151,11 @@ int xen_kexec_unload(uint64_t kexec_flags)
>       return -1;
>  }
>
> +int xen_kexec_status(uint64_t kexec_flags)
> +{
> +     return -1;
> +}
> +
>  void xen_kexec_exec(void)
>  {
>  }
> diff --git a/kexec/kexec.8 b/kexec/kexec.8
> index 4d0c1d1..f4b39a6 100644
> --- a/kexec/kexec.8
> +++ b/kexec/kexec.8
> @@ -107,6 +107,12 @@ command:
>  .B \-d\ (\-\-debug)
>  Enable debugging messages.
>  .TP
> +.B \-S\ (\-\-status)
> +Return 0 if the type (by default crash) is loaded. Can be used in conjuction
> +with -l or -p to toggle the type. Note this option supersedes other options
> +and it will
> +.BR not\ load\ or\ unload\ the\ kernel.

Same as above. I think that you have missed my earlier comments.
I suppose that you can join "+and it will" and "+.BR not\ load\ or\
unload\ the\ kernel." into one line.

> +.TP
>  .B \-e\ (\-\-exec)
>  Run the currently loaded kernel. Note that it will reboot into the loaded 
> kernel without calling shutdown(8).
>  .TP
> diff --git a/kexec/kexec.c b/kexec/kexec.c
> index 500e5a9..defbbe3 100644
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -51,6 +51,9 @@
>  #include "kexec-lzma.h"
>  #include <arch/options.h>
>
> +#define KEXEC_LOADED_PATH "/sys/kernel/kexec_loaded"
> +#define KEXEC_CRASH_LOADED_PATH "/sys/kernel/kexec_crash_loaded"
> +
>  unsigned long long mem_min = 0;
>  unsigned long long mem_max = ULONG_MAX;
>  static unsigned long kexec_flags = 0;
> @@ -58,6 +61,8 @@ static unsigned long kexec_flags = 0;
>  static unsigned long kexec_file_flags = 0;
>  int kexec_debug = 0;
>
> +static int kexec_loaded(const char *file);
> +

Why are you shuffling this...

>  void dbgprint_mem_range(const char *prefix, struct memory_range *mr, int 
> nr_mr)
>  {
>       int i;
> @@ -890,8 +895,6 @@ static int my_exec(void)
>       return -1;
>  }
>
> -static int kexec_loaded(void);
> -

...and this. Could not you leave this forward declaration here (of
course with arg change)? Or is it possible to drop it at all?

Daniel

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