[PATCH] xl: Check for dependencies in xl

[PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

If I fail to start xencommons before starting a vm, I get a non
obvious "failed to free memory for the domain" error!  This patch
checks for dependencies when xl is invoked.

Note:  Currently I am checking for xenstore daemon only.  More checks
could be added if necessary.  Also, I am performing the check
irrespective of the kind of command being invoked through xl.
Example. xl info, list etc.  doesn't require that xenstored be
running.  But expecting xenstored be running before invoking the
toolstack seem like a reasonable expectation in general.  Please
consider this for inclusion or feel free to tweak it as necessary.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r ce208811f540 tools/libxl/xl.c
--- a/tools/libxl/xl.c  Thu Jan 13 01:26:44 2011 +0000
+++ b/tools/libxl/xl.c  Thu Jan 13 11:26:35 2011 -0500
@@ -74,6 +74,27 @@ static void parse_global_config(const ch

     xlu_cfg_destroy(config);
 }
+
+static int check_dependencies(void)
+{
+    struct xs_handle *xsh;
+
+    xsh = xs_daemon_open();
+    if ( !xsh )
+    {
+        fprintf(stderr, "xs_daemon_open failed!\n");
+        return 0;
+    }
+
+    if ( xs_read(xsh, XBT_NULL, "/local/domain/0/name", NULL) == NULL )
+    {
+        fprintf(stderr, "Xenstore daemon is not running or not
initialized properly!\n");
+        return 0;
+    }
+
+    xs_daemon_close(xsh);
+    return 1;
+}

 int main(int argc, char **argv)
 {
@@ -103,6 +124,9 @@ int main(int argc, char **argv)
         exit(1);
     }
     opterr = 0;
+
+    if ( !check_dependencies() )
+        exit(1);

     logger = xtl_createlogger_stdiostream(stderr, minmsglevel,  0);
     if (!logger) exit(1);

Re: [PATCH] xl: Check for dependencies in xl

[Ian Campbell]

On Thu, 2011-01-13 at 16:57 +0000, Kamala Narasimhan wrote: 
> If I fail to start xencommons before starting a vm, I get a non
> obvious "failed to free memory for the domain" error!  This patch
> checks for dependencies when xl is invoked.
> 
> Note:  Currently I am checking for xenstore daemon only.  More checks
> could be added if necessary.  Also, I am performing the check
> irrespective of the kind of command being invoked through xl.
> Example. xl info, list etc.  doesn't require that xenstored be
> running.  But expecting xenstored be running before invoking the
> toolstack seem like a reasonable expectation in general.  Please
> consider this for inclusion or feel free to tweak it as necessary.

Having just tripped over this on a new install where xenstored wasn't
started at the default runlevel I like this idea.

> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> diff -r ce208811f540 tools/libxl/xl.c
> --- a/tools/libxl/xl.c  Thu Jan 13 01:26:44 2011 +0000
> +++ b/tools/libxl/xl.c  Thu Jan 13 11:26:35 2011 -0500
> @@ -74,6 +74,27 @@ static void parse_global_config(const ch
> 
>      xlu_cfg_destroy(config);
>  }
> +
> +static int check_dependencies(void)
> +{
> +    struct xs_handle *xsh;
> +
> +    xsh = xs_daemon_open();

This function is deprecated, please use xs_open. In addition this would
fail if you are running xenstored in a different domain to xl (e.g. a
stub-xenstored-dom).

Unfortunately the problem with opening a connection to xenstored in a
way which can cope with a remote xenstored is that it can block
indefinitely if the other end is not available.

One idea might be to simply check for the xenstored pidfile instead.
IIRC the prototypes of stub-xenstored also dropped a "domidfile" which
could also be checked in that case.

Otherwise I suspect the correct fix might involve a fix to
the /proc/xen/xenbus kernel driver so it doesn't block in this way or
has a timeout etc.

Perhaps this is more a theoretical concern, since stubdom xenstored
never really progressed past the prototype stage. It would be nice not
to add more things which would need fixing up for that case though.

> @@ -103,6 +124,9 @@ int main(int argc, char **argv)
>          exit(1);
>      }
>      opterr = 0;
> +
> +    if ( !check_dependencies() )
> +        exit(1);
> 
>      logger = xtl_createlogger_stdiostream(stderr, minmsglevel,  0);
>      if (!logger) exit(1);

The next thing after this xtl_createlogger_stdiostream(...) is a call to
libxl_ctx_init which will try and open the xenstore in the same way and
has error logging already so perhaps we should improve on that instead
of adding a separate check?

Ian.

Re: [PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

On Fri, Jan 14, 2011 at 12:51 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> +
>> +static int check_dependencies(void)
>> +{
>> +    struct xs_handle *xsh;
>> +
>> +    xsh = xs_daemon_open();
>
> This function is deprecated, please use xs_open. In addition this would
> fail if you are running xenstored in a different domain to xl (e.g. a
> stub-xenstored-dom).
>
> Unfortunately the problem with opening a connection to xenstored in a
> way which can cope with a remote xenstored is that it can block
> indefinitely if the other end is not available.
>
> One idea might be to simply check for the xenstored pidfile instead.
> IIRC the prototypes of stub-xenstored also dropped a "domidfile" which
> could also be checked in that case.
>

Will switch to checking for the pid file then.

> Otherwise I suspect the correct fix might involve a fix to
> the /proc/xen/xenbus kernel driver so it doesn't block in this way or
> has a timeout etc.
>
> Perhaps this is more a theoretical concern, since stubdom xenstored
> never really progressed past the prototype stage. It would be nice not
> to add more things which would need fixing up for that case though.
>
>> @@ -103,6 +124,9 @@ int main(int argc, char **argv)
>>          exit(1);
>>      }
>>      opterr = 0;
>> +
>> +    if ( !check_dependencies() )
>> +        exit(1);
>>
>>      logger = xtl_createlogger_stdiostream(stderr, minmsglevel,  0);
>>      if (!logger) exit(1);
>
> The next thing after this xtl_createlogger_stdiostream(...) is a call to
> libxl_ctx_init which will try and open the xenstore in the same way and
> has error logging already so perhaps we should improve on that instead
> of adding a separate check?
>
>

Aren't we making the following assumptions in doing so -

1)  xtl_createlogger_stdiostream would always be the first function
called that uses xenstore.
2)  There aren't likely to be other dependency checks to be added in
future that might warrant a separate function to perform such checks?

Kamala

Re: [PATCH] xl: Check for dependencies in xl

[Ian Campbell]

On Fri, 2011-01-14 at 18:57 +0000, Kamala Narasimhan wrote:
> 
> > The next thing after this xtl_createlogger_stdiostream(...) is a
> call to
> > libxl_ctx_init which will try and open the xenstore in the same way
> and
> > has error logging already so perhaps we should improve on that
> instead
> > of adding a separate check?
> >
> >
> 
> Aren't we making the following assumptions in doing so -
> 
> 1)  xtl_createlogger_stdiostream would always be the first function
> called that uses xenstore.

You mean "libxl_ctx_init would always be..."?

In that case yes since that it is the function contains opens the
connection to xenstore so it must be called first. A libxl user
shouldn't need to see or use xenstore directly itself since libxl
encapsulates this.

xtl_createlogger_stdiostream does/can not use xenstore since it comes
from libxc which is beneath (or perhaps alongside) libxenstore in the
software stack.

> 2)  There aren't likely to be other dependency checks to be added in
> future that might warrant a separate function to perform such checks? 

It's not clear what other checks there will end up being but those which
are due to libxl's dependencies (e.g. xenstore) should be in a libxl
function. This could be a separate libxl_check_dependencies() but I
can't see any reason not to do it in libxl_ctx_init().

If xl has some dependency of its own independent of libxl which it is
useful to check then it would be appropriate to add those checks to xl
itself.

Ian.

Re: [PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

> You mean "libxl_ctx_init would always be..."?
>

Yes, sorry.

> In that case yes since that it is the function contains opens the
> connection to xenstore so it must be called first. A libxl user
> shouldn't need to see or use xenstore directly itself since libxl
> encapsulates this.
>
> xtl_createlogger_stdiostream does/can not use xenstore since it comes
> from libxc which is beneath (or perhaps alongside) libxenstore in the
> software stack.
>
>> 2)  There aren't likely to be other dependency checks to be added in
>> future that might warrant a separate function to perform such checks?
>
> It's not clear what other checks there will end up being but those which
> are due to libxl's dependencies (e.g. xenstore) should be in a libxl
> function. This could be a separate libxl_check_dependencies() but I
> can't see any reason not to do it in libxl_ctx_init().
>
> If xl has some dependency of its own independent of libxl which it is
> useful to check then it would be appropriate to add those checks to xl
> itself.
>

Since this toolstack is new to me, as you might have observed from the
previous threads, I didn't differentiate between libxl/xl etc. (in
terms of where the changes end up) while writing those patches and
piling it all up as one unit :)   Now that I understand, I will make
the necessary changes.

Kamala

Re: [PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

Revised patch based on your comments.  Please let me know if there are
further suggestions.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Wed Jan 19 13:42:24 2011 -0500
@@ -38,13 +38,24 @@

 #define PAGE_TO_MEMKB(pages) ((pages) * 4)

+static char const xenstore_pid_file[] = "/var/run/xenstore.pid";
+
 int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
 {
+    struct stat stat_buf;
+
     if (version != LIBXL_VERSION)
         return ERROR_VERSION;
     memset(ctx, 0, sizeof(libxl_ctx));
     ctx->lg = lg;
     memset(&ctx->version_info, 0, sizeof(libxl_version_info));
+
+    if ( stat(xenstore_pid_file, &stat_buf) != 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+            "Is xenstore daemon running?\nStat on file %s returned -
\"%s\".\n",
+            xenstore_pid_file, strerror(errno));
+        return ERROR_FAIL;
+    }

     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {

Re: [PATCH] xl: Check for dependencies in xl

[Stefano Stabellini]

On Wed, 19 Jan 2011, Kamala Narasimhan wrote:
> Revised patch based on your comments.  Please let me know if there are
> further suggestions.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> diff -r fe8a177ae9cb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
> +++ b/tools/libxl/libxl.c       Wed Jan 19 13:42:24 2011 -0500
> @@ -38,13 +38,24 @@
> 
>  #define PAGE_TO_MEMKB(pages) ((pages) * 4)
> 
> +static char const xenstore_pid_file[] = "/var/run/xenstore.pid";
> +

I think is a good idea to #define XENSTORE_PID "/var/run/xenstore.pid"
in libxl_internal.h

Re: [PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

My preference would to use static const over preprocessor definitions
but I also don't mind conformance if that is why you are suggesting
this change.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Thu Jan 20 08:58:06 2011 -0500
@@ -40,11 +40,20 @@

 int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
 {
+    struct stat stat_buf;
+
     if (version != LIBXL_VERSION)
         return ERROR_VERSION;
     memset(ctx, 0, sizeof(libxl_ctx));
     ctx->lg = lg;
     memset(&ctx->version_info, 0, sizeof(libxl_version_info));
+
+    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+            "Is xenstore daemon running?\nStat on file %s returned -
\"%s\".\n",
+            XENSTORE_PID_FILE, strerror(errno));
+        return ERROR_FAIL;
+    }

     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {
diff -r fe8a177ae9cb tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h      Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl_internal.h      Thu Jan 20 08:58:06 2011 -0500
@@ -104,6 +104,7 @@ typedef struct {
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
 #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
+#define XENSTORE_PID_FILE      "/var/run/xenstore.pid"

 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01

Re: [PATCH] xl: Check for dependencies in xl

[Stefano Stabellini]

On Thu, 20 Jan 2011, Kamala Narasimhan wrote:
> My preference would to use static const over preprocessor definitions
> but I also don't mind conformance if that is why you are suggesting
> this change.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Re: [PATCH] xl: Check for dependencies in xl

[Ian Jackson]

Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Check for dependencies in xl"):
> My preference would to use static const over preprocessor definitions
> but I also don't mind conformance if that is why you are suggesting
> this change.

Thanks, I would have applied this patch, but:

> +
> +    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +            "Is xenstore daemon running?\nStat on file %s returned -
> \"%s\".\n",
> +            XENSTORE_PID_FILE, strerror(errno));
> +        return ERROR_FAIL;
> +    }

This part has been word-wrapped.  You need to send patches in a way
which does not word-wrap them.

And ideally you should probably use C99 string concatenation to split
the string so as to avoid the long line.

Thanks,
Ian.

Re: [PATCH] xl: Check for dependencies in xl

[Stefano Stabellini]

On Fri, 21 Jan 2011, Ian Jackson wrote:
> Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Check for dependencies in xl"):
> > My preference would to use static const over preprocessor definitions
> > but I also don't mind conformance if that is why you are suggesting
> > this change.
> 
> Thanks, I would have applied this patch, but:
> 
> > +
> > +    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
> > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> > +            "Is xenstore daemon running?\nStat on file %s returned -
> > \"%s\".\n",
> > +            XENSTORE_PID_FILE, strerror(errno));
> > +        return ERROR_FAIL;
> > +    }
> 
> This part has been word-wrapped.  You need to send patches in a way
> which does not word-wrap them.

you can find very useful the document:

Documentation/email-clients.txt

in any linux kernel tree

Re: [PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

Stefano Stabellini wrote:
> On Fri, 21 Jan 2011, Ian Jackson wrote:
> you can find very useful the document:
> 
> Documentation/email-clients.txt
> 
> in any linux kernel tree

Yes, it was useful indeed.  Thanks.

Kamala

Re: [PATCH] xl: Check for dependencies in xl

[Gianni Tedesco]

On Wed, 2011-01-19 at 18:46 +0000, Kamala Narasimhan wrote:
> Revised patch based on your comments.  Please let me know if there are
> further suggestions.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> diff -r fe8a177ae9cb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
> +++ b/tools/libxl/libxl.c       Wed Jan 19 13:42:24 2011 -0500
> @@ -38,13 +38,24 @@
> 
>  #define PAGE_TO_MEMKB(pages) ((pages) * 4)
> 
> +static char const xenstore_pid_file[] = "/var/run/xenstore.pid";
> +
>  int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
>  {
> +    struct stat stat_buf;
> +
>      if (version != LIBXL_VERSION)
>          return ERROR_VERSION;
>      memset(ctx, 0, sizeof(libxl_ctx));
>      ctx->lg = lg;
>      memset(&ctx->version_info, 0, sizeof(libxl_version_info));
> +
> +    if ( stat(xenstore_pid_file, &stat_buf) != 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +            "Is xenstore daemon running?\nStat on file %s returned -
> \"%s\".\n",
> +            xenstore_pid_file, strerror(errno));
> +        return ERROR_FAIL;
> +    }

NAK

Actually I think this type of check should go after the xenstore open
has failed, just as an informational/hint type of thing IF it fails.
Rather than applying what is really a heuristic and making it CAUSE a
failure.

Gianni

Re: [PATCH] xl: Check for dependencies in xl

[Kamala Narasimhan]

> 
> NAK
> 
> Actually I think this type of check should go after the xenstore open
> has failed, just as an informational/hint type of thing IF it fails.
> Rather than applying what is really a heuristic and making it CAUSE a
> failure.
> 
Except xs_daemon_open called within libxl_ctx_init does not fail when xenstored is not running!  We get this non-obvious error message - "failed to free memory for the domain" at a later point.  This patch was an attempt to make the "xenstored not running" issue more obvious.  

So, without this check we might have to end up chasing xs_* calls that are bound to fail because of using the non null xs_handle returned by xs_daemon_open and apply this heuristic upon its failure to make the issue more obvious.  Given this info, do you still think we shouldn't perform this check?  It would certainly make troubleshooting easier.

Alternately, we could investigate why xs_daemon_open is returning a non null handle when xenstored is not running.

Kamala

Re: [PATCH] xl: Check for dependencies in xl

[Gianni Tedesco]

On Mon, 2011-01-24 at 17:26 +0000, Kamala Narasimhan wrote:
> > 
> > NAK
> > 
> > Actually I think this type of check should go after the xenstore open
> > has failed, just as an informational/hint type of thing IF it fails.
> > Rather than applying what is really a heuristic and making it CAUSE a
> > failure.
> > 
> Except xs_daemon_open called within libxl_ctx_init does not fail when
> xenstored is not running!  We get this non-obvious error message -
> "failed to free memory for the domain" at a later point.  This patch
> was an attempt to make the "xenstored not running" issue more
> obvious.  

Oh yes, I didn't pick up on that.

> So, without this check we might have to end up chasing xs_* calls that
> are bound to fail because of using the non null xs_handle returned by
> xs_daemon_open and apply this heuristic upon its failure to make the
> issue more obvious.  Given this info, do you still think we shouldn't
> perform this check?  It would certainly make troubleshooting easier.

I think the idea of having such heuristic checks is a good one. Error
messages can be a bit baffling.

> Alternately, we could investigate why xs_daemon_open is returning a
> non null handle when xenstored is not running.

Sounds like a good idea since, if that is the case, how can a caller
know whether it really got a xenstore connection or not? Maybe we can
just try and ls / and that will fail?

I would like to hear Ian Campbells opinion on this but he's away until
Wednesday I think.

Gianni

Re: [PATCH] xl: Check for dependencies in xl

[Ian Jackson]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] xl: Check for dependencies in xl"):
> I think the idea of having such heuristic checks is a good one. Error
> messages can be a bit baffling.

I think it would be far better to have a check that checked directly
for the thing we are going to use.  But in this case that's not
available without a lot more work.

So I have applied Kamala's patch.

Ian.