[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2] libxl: prevent xl from running if xend is running.



On Tue, 2012-04-24 at 19:05 +0100, Roger Pau Monne wrote:
> 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.
> 
> Changes since v1:
> 
>  * Add documentation to xl man page.
> 
>  * Permit the execution of commands that don't modify anything.
> 
>  * Indent error message.
> 
> Cc: george.dunlap@xxxxxxxxxxxxx
> Cc: ian.jackson@xxxxxxxxxxxxx
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> ---
>  docs/man/xl.pod.1         |    6 ++
>  tools/libxl/xl.c          |   22 +++++++-
>  tools/libxl/xl.h          |    1 +
>  tools/libxl/xl_cmdimpl.c  |    4 +-
>  tools/libxl/xl_cmdtable.c |  132 ++++++++++++++++++++++----------------------
>  5 files changed, 96 insertions(+), 69 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index e5324fb..e829697 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -75,6 +75,12 @@ Verbose.
> 
>  Dry run: do not actually execute the command.
> 
> +=item B<-f>
> +
> +Force execution: xl will refuse to run some commands if it detects that xend 
> is
> +also running, this option will force the execution of those commands, even
> +though it is unsafe.
> +
>  =back
> 
>  =head1 DOMAIN SUBCOMMANDS
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 2b14814..720bb66 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);
> @@ -162,6 +169,19 @@ int main(int argc, char **argv)
>              ret = 1;
>              goto xit;
>          }
> +        if (cspec->modifies) {

Need to handle -N (dry-run) too? That will turn a modifying command into
a non-modifying one.

Do we now have 3 classes of commands which could be represented with an
enum rather than a pair of bools?

      * Purely introspective commands, only query data, do not change
        any state, -N and -f mean nothing.
      * Commands which modify state, but which have a "dry-run" mode. -N
        activates dry-run, -f only means something if !dry-run
      * Commands which modify state, but which do not support a dry-run
        mode, -N is not available, -f does what you'd expect.

> +            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.\n"
> +"If you still want to force the execution of xl please use the -f\n"
> +"option.\n"
> +                            );
> +                    ret = 1;
> +                    goto xit;
> +                }
> +            }
> +        }
>          ret = cspec->cmd_impl(argc, argv);
>      } else if (!strcmp(cmd, "help")) {
>          help(argv[1]);
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 0a3d628..5ddd2da 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -21,6 +21,7 @@ struct cmd_spec {
>      char *cmd_name;
>      int (*cmd_impl)(int argc, char **argv);
>      int can_dryrun;
> +    int modifies;
>      char *cmd_desc;
>      char *cmd_usage;
>      char *cmd_option;
> 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,
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index f461a2a..5509126 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -19,7 +19,7 @@
> 
>  struct cmd_spec cmd_table[] = {
>      { "create",
> -      &main_create, 1,
> +      &main_create, 1, 1,
>        "Create a domain from config file <filename>",
>        "<ConfigFile> [options] [vars]",
>        "-h                      Print this help.\n"
> @@ -33,7 +33,7 @@ struct cmd_spec cmd_table[] = {
>        "-e                      Do not wait in the background for the death 
> of the domain."
>      },
>      { "config-update",
> -      &main_config_update, 1,
> +      &main_config_update, 1, 1,
>        "Update a running domain's saved configuration, used when rebuilding "
>        "the domain after reboot",
>        "<Domain> <ConfigFile> [options] [vars]",
> @@ -42,7 +42,7 @@ struct cmd_spec cmd_table[] = {
>        "-d                      Enable debug messages.\n"
>      },
>      { "list",
> -      &main_list, 0,
> +      &main_list, 0, 0,
>        "List information about all/some domains",
>        "[options] [Domain]\n",
>        "-l, --long              Output all VM details\n"
> @@ -50,12 +50,12 @@ struct cmd_spec cmd_table[] = {
>        "-Z, --context           Prints out security context"
>      },
>      { "destroy",
> -      &main_destroy, 0,
> +      &main_destroy, 0, 1,
>        "Terminate a domain immediately",
>        "<Domain>",
>      },
>      { "shutdown",
> -      &main_shutdown, 0,
> +      &main_shutdown, 0, 1,
>        "Issue a shutdown signal to a domain",
>        "[options] <Domain>",
>        "-h                      Print this help.\n"
> @@ -64,7 +64,7 @@ struct cmd_spec cmd_table[] = {
>        "-w                      Wait for guest to shutdown.\n"
>      },
>      { "reboot",
> -      &main_reboot, 0,
> +      &main_reboot, 0, 1,
>        "Issue a reboot signal to a domain",
>        "[options] <Domain>",
>        "-h                      Print this help.\n"
> @@ -72,44 +72,44 @@ struct cmd_spec cmd_table[] = {
>        "                        no PV drivers.\n"
>      },
>      { "pci-attach",
> -      &main_pciattach, 0,
> +      &main_pciattach, 0, 1,
>        "Insert a new pass-through pci device",
>        "<Domain> <BDF> [Virtual Slot]",
>      },
>      { "pci-detach",
> -      &main_pcidetach, 0,
> +      &main_pcidetach, 0, 1,
>        "Remove a domain's pass-through pci device",
>        "<Domain> <BDF>",
>      },
>      { "pci-list",
> -      &main_pcilist, 0,
> +      &main_pcilist, 0, 0,
>        "List pass-through pci devices for a domain",
>        "<Domain>",
>      },
>      { "pci-list-assignable-devices",
> -      &main_pcilist_assignable, 0,
> +      &main_pcilist_assignable, 0, 0,
>        "List all the assignable pci devices",
>        "",
>      },
>      { "pause",
> -      &main_pause, 0,
> +      &main_pause, 0, 1,
>        "Pause execution of a domain",
>        "<Domain>",
>      },
>      { "unpause",
> -      &main_unpause, 0,
> +      &main_unpause, 0, 1,
>        "Unpause a paused domain",
>        "<Domain>",
>      },
>      { "console",
> -      &main_console, 0,
> +      &main_console, 0, 0,
>        "Attach to domain's console",
>        "[options] <Domain>\n"
>        "-t <type>       console type, pv or serial\n"
>        "-n <number>     console number"
>      },
>      { "vncviewer",
> -      &main_vncviewer, 0,
> +      &main_vncviewer, 0, 0,
>        "Attach to domain's VNC server.",
>        "[options] <Domain>\n"
>        "--autopass               Pass VNC password to viewer via stdin and\n"
> @@ -117,14 +117,14 @@ struct cmd_spec cmd_table[] = {
>        "--vncviewer-autopass     (consistency alias for --autopass)"
>      },
>      { "save",
> -      &main_save, 0,
> +      &main_save, 0, 1,
>        "Save a domain state to restore later",
>        "[options] <Domain> <CheckpointFile> [<ConfigFile>]",
>        "-h  Print this help.\n"
>        "-c  Leave domain running after creating the snapshot."
>      },
>      { "migrate",
> -      &main_migrate, 0,
> +      &main_migrate, 0, 1,
>        "Save a domain state to restore later",
>        "[options] <Domain> <host>",
>        "-h              Print this help.\n"
> @@ -136,12 +136,12 @@ struct cmd_spec cmd_table[] = {
>        "                of the domain."
>      },
>      { "dump-core",
> -      &main_dump_core, 0,
> +      &main_dump_core, 0, 1,
>        "Core dump a domain",
>        "<Domain> <filename>"
>      },
>      { "restore",
> -      &main_restore, 0,
> +      &main_restore, 0, 1,
>        "Restore a domain from a saved state",
>        "[options] [<ConfigFile>] <CheckpointFile>",
>        "-h  Print this help.\n"
> @@ -150,68 +150,68 @@ struct cmd_spec cmd_table[] = {
>        "-d  Enable debug messages."
>      },
>      { "migrate-receive",
> -      &main_migrate_receive, 0,
> +      &main_migrate_receive, 0, 1,
>        "Restore a domain from a saved state",
>        "- for internal use only",
>      },
>      { "cd-insert",
> -      &main_cd_insert, 0,
> +      &main_cd_insert, 0, 1,
>        "Insert a cdrom into a guest's cd drive",
>        "<Domain> <VirtualDevice> <type:path>",
>      },
>      { "cd-eject",
> -      &main_cd_eject, 0,
> +      &main_cd_eject, 0, 1,
>        "Eject a cdrom from a guest's cd drive",
>        "<Domain> <VirtualDevice>",
>      },
>      { "mem-max",
> -      &main_memmax, 0,
> +      &main_memmax, 0, 1,
>        "Set the maximum amount reservation for a domain",
>        "<Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
>      },
>      { "mem-set",
> -      &main_memset, 0,
> +      &main_memset, 0, 1,
>        "Set the current memory usage for a domain",
>        "<Domain> <MemMB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
>      },
>      { "button-press",
> -      &main_button_press, 0,
> +      &main_button_press, 0, 1,
>        "Indicate an ACPI button press to the domain",
>        "<Domain> <Button>",
>        "<Button> may be 'power' or 'sleep'."
>      },
>      { "vcpu-list",
> -      &main_vcpulist, 0,
> +      &main_vcpulist, 0, 0,
>        "List the VCPUs for all/some domains",
>        "[Domain, ...]",
>      },
>      { "vcpu-pin",
> -      &main_vcpupin, 0,
> +      &main_vcpupin, 0, 1,
>        "Set which CPUs a VCPU can use",
>        "<Domain> <VCPU|all> <CPUs|all>",
>      },
>      { "vcpu-set",
> -      &main_vcpuset, 0,
> +      &main_vcpuset, 0, 1,
>        "Set the number of active VCPUs allowed for the domain",
>        "<Domain> <vCPUs>",
>      },
>      { "list-vm",
> -      &main_list_vm, 0,
> +      &main_list_vm, 0, 0,
>        "List the VMs,without DOM0",
>        "",
>      },
>      { "info",
> -      &main_info, 0,
> +      &main_info, 0, 0,
>        "Get information about Xen host",
>        "-n, --numa         List host NUMA topology information",
>      },
>      { "sharing",
> -      &main_sharing, 0,
> +      &main_sharing, 0, 0,
>        "Get information about page sharing",
>        "[Domain]",
>      },
>      { "sched-credit",
> -      &main_sched_credit, 0,
> +      &main_sched_credit, 0, 1,
>        "Get/set credit scheduler parameters",
>        "[-d <Domain> [-w[=WEIGHT]|-c[=CAP]]] [-s [-t TSLICE] [-r RATELIMIT]] 
> [-p CPUPOOL]",
>        "-d DOMAIN, --domain=DOMAIN        Domain to modify\n"
> @@ -223,7 +223,7 @@ struct cmd_spec cmd_table[] = {
>        "-p CPUPOOL, --cpupool=CPUPOOL     Restrict output to CPUPOOL"
>      },
>      { "sched-credit2",
> -      &main_sched_credit2, 0,
> +      &main_sched_credit2, 0, 1,
>        "Get/set credit2 scheduler parameters",
>        "[-d <Domain> [-w[=WEIGHT]]] [-p CPUPOOL]",
>        "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
> @@ -231,7 +231,7 @@ struct cmd_spec cmd_table[] = {
>        "-p CPUPOOL, --cpupool=CPUPOOL  Restrict output to CPUPOOL"
>      },
>      { "sched-sedf",
> -      &main_sched_sedf, 0,
> +      &main_sched_sedf, 0, 1,
>        "Get/set sedf scheduler parameters",
>        "[options]",
>        "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
> @@ -247,109 +247,109 @@ struct cmd_spec cmd_table[] = {
>        "-c CPUPOOL, --cpupool=CPUPOOL  Restrict output to CPUPOOL"
>      },
>      { "domid",
> -      &main_domid, 0,
> +      &main_domid, 0, 0,
>        "Convert a domain name to domain id",
>        "<DomainName>",
>      },
>      { "domname",
> -      &main_domname, 0,
> +      &main_domname, 0, 0,
>        "Convert a domain id to domain name",
>        "<DomainId>",
>      },
>      { "rename",
> -      &main_rename, 0,
> +      &main_rename, 0, 1,
>        "Rename a domain",
>        "<Domain> <NewDomainName>",
>      },
>      { "trigger",
> -      &main_trigger, 0,
> +      &main_trigger, 0, 1,
>        "Send a trigger to a domain",
>        "<Domain> <nmi|reset|init|power|sleep|s3resume> [<VCPU>]",
>      },
>      { "sysrq",
> -      &main_sysrq, 0,
> +      &main_sysrq, 0, 1,
>        "Send a sysrq to a domain",
>        "<Domain> <letter>",
>      },
>      { "debug-keys",
> -      &main_debug_keys, 0,
> +      &main_debug_keys, 0, 1,
>        "Send debug keys to Xen",
>        "<Keys>",
>      },
>      { "dmesg",
> -      &main_dmesg, 0,
> +      &main_dmesg, 0, 0,
>        "Read and/or clear dmesg buffer",
>        "[-c]",
>        "  -c                        Clear dmesg buffer as well as printing 
> it",
>      },
>      { "top",
> -      &main_top, 0,
> +      &main_top, 0, 0,
>        "Monitor a host and the domains in real time",
>        "",
>      },
>      { "network-attach",
> -      &main_networkattach, 0,
> +      &main_networkattach, 0, 1,
>        "Create a new virtual network device",
>        "<Domain> [type=<type>] [mac=<mac>] [bridge=<bridge>] "
>        "[ip=<ip>] [script=<script>] [backend=<BackDomain>] [vifname=<name>] "
>        "[rate=<rate>] [model=<model>] [accel=<accel>]",
>      },
>      { "network-list",
> -      &main_networklist, 0,
> +      &main_networklist, 0, 0,
>        "List virtual network interfaces for a domain",
>        "<Domain(s)>",
>      },
>      { "network-detach",
> -      &main_networkdetach, 0,
> +      &main_networkdetach, 0, 1,
>        "Destroy a domain's virtual network device",
>        "<Domain> <DevId|mac>",
>      },
>      { "block-attach",
> -      &main_blockattach, 1,
> +      &main_blockattach, 1, 1,
>        "Create a new virtual block device",
>        "<Domain> <disk-spec-component(s)>...",
>      },
>      { "block-list",
> -      &main_blocklist, 0,
> +      &main_blocklist, 0, 0,
>        "List virtual block devices for a domain",
>        "<Domain(s)>",
>      },
>      { "block-detach",
> -      &main_blockdetach, 0,
> +      &main_blockdetach, 0, 1,
>        "Destroy a domain's virtual block device",
>        "<Domain> <DevId>",
>      },
>      { "uptime",
> -      &main_uptime, 0,
> +      &main_uptime, 0, 0,
>        "Print uptime for all/some domains",
>        "[-s] [Domain]",
>      },
>      { "tmem-list",
> -      &main_tmem_list, 0,
> +      &main_tmem_list, 0, 0,
>        "List tmem pools",
>        "[-l] [<Domain>|-a]",
>        "  -l                             List tmem stats",
>      },
>      { "tmem-freeze",
> -      &main_tmem_freeze, 0,
> +      &main_tmem_freeze, 0, 1,
>        "Freeze tmem pools",
>        "[<Domain>|-a]",
>        "  -a                             Freeze all tmem",
>      },
>      { "tmem-destroy",
> -      &main_tmem_destroy, 0,
> +      &main_tmem_destroy, 0, 1,
>        "Destroy tmem pools",
>        "[<Domain>|-a]",
>        "  -a                             Destroy all tmem",
>      },
>      { "tmem-thaw",
> -      &main_tmem_thaw, 0,
> +      &main_tmem_thaw, 0, 1,
>        "Thaw tmem pools",
>        "[<Domain>|-a]",
>        "  -a                             Thaw all tmem",
>      },
>      { "tmem-set",
> -      &main_tmem_set, 0,
> +      &main_tmem_set, 0, 1,
>        "Change tmem settings",
>        "[<Domain>|-a] [-w[=WEIGHT]|-c[=CAP]|-p[=COMPRESS]]",
>        "  -a                             Operate on all tmem\n"
> @@ -358,7 +358,7 @@ struct cmd_spec cmd_table[] = {
>        "  -p COMPRESS                    Compress (int)",
>      },
>      { "tmem-shared-auth",
> -      &main_tmem_shared_auth, 0,
> +      &main_tmem_shared_auth, 0, 1,
>        "De/authenticate shared tmem pool",
>        "[<Domain>|-a] [-u[=UUID] [-A[=AUTH]",
>        "  -a                             Authenticate for all tmem pools\n"
> @@ -367,12 +367,12 @@ struct cmd_spec cmd_table[] = {
>        "  -A AUTH                        0=auth,1=deauth",
>      },
>      { "tmem-freeable",
> -      &main_tmem_freeable, 0,
> +      &main_tmem_freeable, 0, 0,
>        "Get information about how much freeable memory (MB) is in-use by 
> tmem",
>        "",
>      },
>      { "cpupool-create",
> -      &main_cpupoolcreate, 1,
> +      &main_cpupoolcreate, 1, 1,
>        "Create a CPU pool based an ConfigFile",
>        "[options] <ConfigFile> [vars]",
>        "-h, --help                   Print this help.\n"
> @@ -381,53 +381,53 @@ struct cmd_spec cmd_table[] = {
>        "                              (deprecated in favour of global -N 
> option)."
>      },
>      { "cpupool-list",
> -      &main_cpupoollist, 0,
> +      &main_cpupoollist, 0, 0,
>        "List CPU pools on host",
>        "[-c|--cpus] [<CPU Pool>]",
>        "-c, --cpus                     Output list of CPUs used by a pool"
>      },
>      { "cpupool-destroy",
> -      &main_cpupooldestroy, 0,
> +      &main_cpupooldestroy, 0, 1,
>        "Deactivates a CPU pool",
>        "<CPU Pool>",
>      },
>      { "cpupool-rename",
> -      &main_cpupoolrename, 0,
> +      &main_cpupoolrename, 0, 1,
>        "Renames a CPU pool",
>        "<CPU Pool> <new name>",
>      },
>      { "cpupool-cpu-add",
> -      &main_cpupoolcpuadd, 0,
> +      &main_cpupoolcpuadd, 0, 1,
>        "Adds a CPU to a CPU pool",
>        "<CPU Pool> <CPU nr>|node:<node nr>",
>      },
>      { "cpupool-cpu-remove",
> -      &main_cpupoolcpuremove, 0,
> +      &main_cpupoolcpuremove, 0, 1,
>        "Removes a CPU from a CPU pool",
>        "<CPU Pool> <CPU nr>|node:<node nr>",
>      },
>      { "cpupool-migrate",
> -      &main_cpupoolmigrate, 0,
> +      &main_cpupoolmigrate, 0, 1,
>        "Moves a domain into a CPU pool",
>        "<Domain> <CPU Pool>",
>      },
>      { "cpupool-numa-split",
> -      &main_cpupoolnumasplit, 0,
> +      &main_cpupoolnumasplit, 0, 1,
>        "Splits up the machine into one CPU pool per NUMA node",
>        "",
>      },
>      { "getenforce",
> -      &main_getenforce, 0,
> +      &main_getenforce, 0, 0,
>        "Returns the current enforcing mode of the Flask Xen security module",
>        "",
>      },
>      { "setenforce",
> -      &main_setenforce, 0,
> +      &main_setenforce, 0, 1,
>        "Sets the current enforcing mode of the Flask Xen security module",
>        "<1|0|Enforcing|Permissive>",
>      },
>      { "loadpolicy",
> -      &main_loadpolicy, 0,
> +      &main_loadpolicy, 0, 1,
>        "Loads a new policy int the Flask Xen security module",
>        "<policy file>",
>      },
> --
> 1.7.7.5 (Apple Git-26)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.