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

[Eric DeVolder <eric.devolder@xxxxxxxxxx>]

On 01/24/2017 01:16 PM, Daniel Kiper wrote:
> 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.
The tabs are not needed for the configure to work properly.

If tabs are needed for readability/style purposes, I will
add them in. There is not any precedent of nesting in
the configure.ac file, so I am unsure what convention is
for this package.

> >  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.
In that file, all dot directives start with the dot in the
first column. I did the same for the .BR in this statement.

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