Xen devel - Apr 2012 - [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.

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)

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);
> +        }
> +    }
> +

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.

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.

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.

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.

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.

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.

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.

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.

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.

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

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.