[PATCH] libxl: prevent xl from running if xend is running.

[PATCH] libxl: prevent xl from running if xend is running.

[Roger Pau Monne]

Prevent xl from doing any operation if xend daemon is running. That prevents
bugs that happened when xl and xend raced to close a domain.

Cc: george.dunlap@eu.citrix.com
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 tools/libxl/xl.c         |   18 +++++++++++++++++-
 tools/libxl/xl_cmdimpl.c |    4 ++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 2b14814..dc434cd 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -32,8 +32,11 @@
 #include "libxlutil.h"
 #include "xl.h"
 
+#define XEND_LOCK { "/var/lock/subsys/xend", "/var/lock/xend" }
+
 xentoollog_logger_stdiostream *logger;
 int dryrun_only;
+int force_execution;
 int autoballoon = 1;
 char *lockfile;
 char *default_vifscript = NULL;
@@ -103,8 +106,9 @@ int main(int argc, char **argv)
     char *config_file;
     void *config_data = 0;
     int config_len = 0;
+    const char *locks[] = XEND_LOCK;
 
-    while ((opt = getopt(argc, argv, "+vN")) >= 0) {
+    while ((opt = getopt(argc, argv, "+vfN")) >= 0) {
         switch (opt) {
         case 'v':
             if (minmsglevel > 0) minmsglevel--;
@@ -112,6 +116,9 @@ int main(int argc, char **argv)
         case 'N':
             dryrun_only = 1;
             break;
+        case 'f':
+            force_execution = 1;
+            break;
         default:
             fprintf(stderr, "unknown global option\n");
             exit(2);
@@ -126,6 +133,15 @@ int main(int argc, char **argv)
     }
     opterr = 0;
 
+    for (int i = 0; i < sizeof(locks)/sizeof(locks[0]); i++) {
+        if (!access(locks[i], F_OK) && !force_execution) {
+            fprintf(stderr, "xend is running, which prevents xl from working "
+                            "correctly. If you still want to force the "
+                            "execution of xl please use the -f option\n");
+            exit(2);
+        }
+    }
+
     logger = xtl_createlogger_stdiostream(stderr, minmsglevel,  0);
     if (!logger) exit(1);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6f4dd09..65bc6d6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1909,7 +1909,7 @@ void help(const char *command)
     struct cmd_spec *cmd;
 
     if (!command || !strcmp(command, "help")) {
-        printf("Usage xl [-vN] <subcommand> [args]\n\n");
+        printf("Usage xl [-vfN] <subcommand> [args]\n\n");
         printf("xl full list of subcommands:\n\n");
         for (i = 0; i < cmdtable_len; i++) {
             printf(" %-19s ", cmd_table[i].cmd_name);
@@ -1920,7 +1920,7 @@ void help(const char *command)
     } else {
         cmd = cmdtable_lookup(command);
         if (cmd) {
-            printf("Usage: xl [-v%s] %s %s\n\n%s.\n\n",
+            printf("Usage: xl [-vf%s] %s %s\n\n%s.\n\n",
                    cmd->can_dryrun ? "N" : "",
                    cmd->cmd_name,
                    cmd->cmd_usage,
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Campbell]

On Mon, 2012-04-23 at 16:11 +0100, Roger Pau Monne wrote:
> @@ -112,6 +116,9 @@ int main(int argc, char **argv)
>          case 'N':
>              dryrun_only = 1;
>              break;
> +        case 'f':
> +            force_execution = 1;
> +            break;
>          default:
>              fprintf(stderr, "unknown global option\n");
>              exit(2);

Needs a matching docs patch to add the option to the manpage. Otherwise:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> @@ -126,6 +133,15 @@ int main(int argc, char **argv)
>      }
>      opterr = 0;
>  
> +    for (int i = 0; i < sizeof(locks)/sizeof(locks[0]); i++) {
> +        if (!access(locks[i], F_OK) && !force_execution) {
> +            fprintf(stderr, "xend is running, which prevents xl from working "
> +                            "correctly. If you still want to force the "
> +                            "execution of xl please use the -f option\n");

You might as well wrap the output text to 80 columns (I didn't count, I
assume this isn't...)

> +            exit(2);
> +        }
> +    }
> +

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH] libxl: prevent xl from running if xend is running."):
> Prevent xl from doing any operation if xend daemon is running. That prevents
> bugs that happened when xl and xend raced to close a domain.

Can we somehow limit this to commands that actually change things ?
Having xl as a diagnostic tool even for xend-based systems is useful.

> +        if (!access(locks[i], F_OK) && !force_execution) {
> +            fprintf(stderr, "xend is running, which prevents xl from working "
> +                            "correctly. If you still want to force the "
> +                            "execution of xl please use the -f option\n");
> +            exit(2);
> +        }

If access fails with an unexpected error code (EACCES? EIO?) this will
blunder on.

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend is running."):
> On Mon, 2012-04-23 at 16:11 +0100, Roger Pau Monne wrote:
...
> > +    for (int i = 0; i < sizeof(locks)/sizeof(locks[0]); i++) {
> > +        if (!access(locks[i], F_OK) && !force_execution) {
> > +            fprintf(stderr, "xend is running, which prevents xl from working "
> > +                            "correctly. If you still want to force the "
> > +                            "execution of xl please use the -f option\n");
> 
> You might as well wrap the output text to 80 columns (I didn't count, I
> assume this isn't...)

It should be.  The best way to do this is probably something like
this:

+            fprintf(stderr,
+ "xend is running, which prevents xl from working correctly.\n"
+ "If you still want to force the execution of xl please use the -f\n"
+ "option\n"
+                    );

(adjust to taste)

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Campbell]

On Tue, 2012-04-24 at 14:17 +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[Xen-devel] [PATCH] libxl: prevent xl from running if xend is running."):
> > Prevent xl from doing any operation if xend daemon is running. That prevents
> > bugs that happened when xl and xend raced to close a domain.
> 
> Can we somehow limit this to commands that actually change things ?
> Having xl as a diagnostic tool even for xend-based systems is useful.

Perhaps a new flag in xl_cmdtable.h? Overriden by -f or -N (dry run).

> > +        if (!access(locks[i], F_OK) && !force_execution) {
> > +            fprintf(stderr, "xend is running, which prevents xl from working "
> > +                            "correctly. If you still want to force the "
> > +                            "execution of xl please use the -f option\n");
> > +            exit(2);
> > +        }
> 
> If access fails with an unexpected error code (EACCES? EIO?) this will
> blunder on.

It'll fail whether the error code is expected or not, won't it?

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> On Tue, 2012-04-24 at 14:17 +0100, Ian Jackson wrote:
> > Can we somehow limit this to commands that actually change things ?
> > Having xl as a diagnostic tool even for xend-based systems is useful.
> 
> Perhaps a new flag in xl_cmdtable.h? Overriden by -f or -N (dry run).

Yes, something like that.

> > > +        if (!access(locks[i], F_OK) && !force_execution) {
> > > +            fprintf(stderr, "xend is running, which prevents xl from working "
> > > +                            "correctly. If you still want to force the "
> > > +                            "execution of xl please use the -f option\n");
> > > +            exit(2);
> > > +        }
> > 
> > If access fails with an unexpected error code (EACCES? EIO?) this will
> > blunder on.
> 
> It'll fail whether the error code is expected or not, won't it?

I think if access fails with EIO, it will return -1, and the if
condition will not be satisfied (!-1 = 0), so the fprintf and exit
will not be taken.

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Campbell]

On Tue, 2012-04-24 at 14:34 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> > On Tue, 2012-04-24 at 14:17 +0100, Ian Jackson wrote:
> > > Can we somehow limit this to commands that actually change things ?
> > > Having xl as a diagnostic tool even for xend-based systems is useful.
> > 
> > Perhaps a new flag in xl_cmdtable.h? Overriden by -f or -N (dry run).
> 
> Yes, something like that.
> 
> > > > +        if (!access(locks[i], F_OK) && !force_execution) {
> > > > +            fprintf(stderr, "xend is running, which prevents xl from working "
> > > > +                            "correctly. If you still want to force the "
> > > > +                            "execution of xl please use the -f option\n");
> > > > +            exit(2);
> > > > +        }
> > > 
> > > If access fails with an unexpected error code (EACCES? EIO?) this will
> > > blunder on.
> > 
> > It'll fail whether the error code is expected or not, won't it?
> 
> I think if access fails with EIO, it will return -1, and the if
> condition will not be satisfied (!-1 = 0), so the fprintf and exit
> will not be taken.

Oh, I was confused because the "good" case here is actually that access
fails...

You could consider this to be a best effort check for xend. IOW we try
and look but if we can't tell then we assume it is not.

It's not terribly robust to just blunder on, but on the other hand being
more robust has a bigger risk of false positives, e.g. failing to start
xl because /var/lock/subsys/ does not exist isn't especially helpful
either (the EACCESS return code doesn't distinguish that
from /var/lock/subsys/xend not existing).

If you are getting EIO or something like that then that's a problem, but
arguably not one which is especially related to whether xend is running
or not and you are bound to get another later on.

You could do a bunch more complex checking (e.g. check the base
directory first) but it seems like overkill for what is supposed to be a
simply sanity check.

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> You could consider this to be a best effort check for xend. IOW we try
> and look but if we can't tell then we assume it is not.

I guess.

> It's not terribly robust to just blunder on, but on the other hand being
> more robust has a bigger risk of false positives, e.g. failing to start
> xl because /var/lock/subsys/ does not exist isn't especially helpful
> either (the EACCESS return code doesn't distinguish that
> from /var/lock/subsys/xend not existing).

EACCES would happen only if the permissions prevented us from
looking.  If /var/lock/subsys doesn't exist we'll get ENOENT, the
"good" error return.

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Campbell]

On Tue, 2012-04-24 at 15:47 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> > You could consider this to be a best effort check for xend. IOW we try
> > and look but if we can't tell then we assume it is not.
> 
> I guess.
> 
> > It's not terribly robust to just blunder on, but on the other hand being
> > more robust has a bigger risk of false positives, e.g. failing to start
> > xl because /var/lock/subsys/ does not exist isn't especially helpful
> > either (the EACCESS return code doesn't distinguish that
> > from /var/lock/subsys/xend not existing).
> 
> EACCES would happen only if the permissions prevented us from
> looking.  If /var/lock/subsys doesn't exist we'll get ENOENT, the
> "good" error return.

Oh, right.

Well, not starting because the perms on /var/lock/subsys are too tight
(e.g. selinux restricting it to initscripts only? unrealistic maybe)
seems unhelpful too.

(I admit this isn't as compelling as my previous example).

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Roger Pau Monne]

Ian Jackson escribió:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
>> On Tue, 2012-04-24 at 14:17 +0100, Ian Jackson wrote:
>>> Can we somehow limit this to commands that actually change things ?
>>> Having xl as a diagnostic tool even for xend-based systems is useful.
>> Perhaps a new flag in xl_cmdtable.h? Overriden by -f or -N (dry run).
>
> Yes, something like that.

Do you mean to add a new "-f" option to each command that performs 
modifications, or modifying the cmd_spec struct to add something like 
"int modifies", and check that before trying to execute the command?

>
>>>> +        if (!access(locks[i], F_OK)&&  !force_execution) {
>>>> +            fprintf(stderr, "xend is running, which prevents xl from working "
>>>> +                            "correctly. If you still want to force the "
>>>> +                            "execution of xl please use the -f option\n");
>>>> +            exit(2);
>>>> +        }
>>> If access fails with an unexpected error code (EACCES? EIO?) this will
>>> blunder on.
>> It'll fail whether the error code is expected or not, won't it?
>
> I think if access fails with EIO, it will return -1, and the if
> condition will not be satisfied (!-1 = 0), so the fprintf and exit
> will not be taken.
>
> Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Jackson]

Roger Pau Monne writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> Ian Jackson escribió:
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> >> On Tue, 2012-04-24 at 14:17 +0100, Ian Jackson wrote:
> >>> Can we somehow limit this to commands that actually change things ?
> >>> Having xl as a diagnostic tool even for xend-based systems is useful.
> >> Perhaps a new flag in xl_cmdtable.h? Overriden by -f or -N (dry run).
> >
> > Yes, something like that.
> 
> Do you mean to add a new "-f" option to each command that performs 
> modifications, or modifying the cmd_spec struct to add something like 
> "int modifies", and check that before trying to execute the command?

The latter.

Ian.

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Roger Pau Monne]

Ian Campbell escribió:
> On Tue, 2012-04-24 at 15:47 +0100, Ian Jackson wrote:
>> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
>>> You could consider this to be a best effort check for xend. IOW we try
>>> and look but if we can't tell then we assume it is not.
>> I guess.
>>
>>> It's not terribly robust to just blunder on, but on the other hand being
>>> more robust has a bigger risk of false positives, e.g. failing to start
>>> xl because /var/lock/subsys/ does not exist isn't especially helpful
>>> either (the EACCESS return code doesn't distinguish that
>>> from /var/lock/subsys/xend not existing).
>> EACCES would happen only if the permissions prevented us from
>> looking.  If /var/lock/subsys doesn't exist we'll get ENOENT, the
>> "good" error return.
>
> Oh, right.
>
> Well, not starting because the perms on /var/lock/subsys are too tight
> (e.g. selinux restricting it to initscripts only? unrealistic maybe)
> seems unhelpful too.
>
> (I admit this isn't as compelling as my previous example).
>
> Ian.
>

What have we decided at the end? Should I do an inverse check, and run 
only if access(...) != 0 && errno == ENOENT?

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

Re: [PATCH] libxl: prevent xl from running if xend is running.

[Ian Jackson]

Roger Pau Monne writes ("Re: [Xen-devel] [PATCH] libxl: prevent xl from running if xend	is running."):
> Ian Campbell escribió:
> > Well, not starting because the perms on /var/lock/subsys are too tight
> > (e.g. selinux restricting it to initscripts only? unrealistic maybe)
> > seems unhelpful too.
...
> What have we decided at the end? Should I do an inverse check, and run 
> only if access(...) != 0 && errno == ENOENT?

Ian and I don't seem to be entirely in agreement, but in this case I
can live with the "best effort" error handling as in your most recent
patch.

Ian.