From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40510) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yxbyy-0006wB-97 for qemu-devel@nongnu.org; Wed, 27 May 2015 10:07:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yxbyu-0001qZ-AV for qemu-devel@nongnu.org; Wed, 27 May 2015 10:06:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yxbyu-0001pe-3x for qemu-devel@nongnu.org; Wed, 27 May 2015 10:06:52 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4RE6pdi006339 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 27 May 2015 10:06:51 -0400 Date: Wed, 27 May 2015 15:06:49 +0100 From: "Richard W.M. Jones" Message-ID: <20150527140649.GO29283@redhat.com> References: <1432732141-33915-1-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1432732141-33915-1-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] watchdog: convert to QemuOpts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, armbru@redhat.com On Wed, May 27, 2015 at 03:09:01PM +0200, Paolo Bonzini wrote: > This makes it possible to specify a watchdog action in a configuration file. > The previous behavior of "-watchdog" is moved to the (implied) "-watchdog > model" suboption. This is already more or less obsolete, since it is possible > to achieve the same effect with "-device", but "-watchdog-action" does not have > an equivalent. > > One alternative implementation is possible, namely to add an "action" > property to each watchdog device. However, boards often have embedded > watchdog devices; even if they currently don't, these should call > watchdog_perform_action() so that they are affected by -watchdog-action. > (This is listed in our BiteSizedTasks wiki page). > > For these boards, "-watchdog action=foo" has two advantages: > > 1) it is much easier to use than a "-global" option, and can be > configured globally for all boards. > > 2) it is harder to add a property to a device than it is to just > s/qemu_system_reset_request/watchdog_perform_action/; in some cases, > the devices are not even qdev-ified at all. The chance of the conversion > happening then would basically be zero if one had to add a property as > a prerequisite. > > Signed-off-by: Paolo Bonzini > --- > tweak brackets in help messages [Eric] > fix missing error message for -watchdog action [Richard] > --- > docs/qdev-device-use.txt | 4 ++++ > qemu-options.hx | 37 +++++++++++++++-------------- > vl.c | 61 +++++++++++++++++++++++++++++++++++------------- > 3 files changed, 69 insertions(+), 33 deletions(-) > > diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt > index 136d271..1906b7d 100644 > --- a/docs/qdev-device-use.txt > +++ b/docs/qdev-device-use.txt > @@ -365,6 +365,10 @@ The old way to define a guest watchdog device is -watchdog DEVNAME. > The new way is -device DEVNAME. For PCI devices, you can add > bus=PCI-BUS,addr=DEVFN to control the PCI device address, as usual. > > +No matter if you use -watchdog or -device to define the guest watchdog > +device, you can specify the watchdog action with "-watchdog-action ACTION" > +or "-watchdog action=ACTION". > + > === Host Device Assignment === > > QEMU supports assigning host PCI devices (qemu-kvm only at this time) > diff --git a/qemu-options.hx b/qemu-options.hx > index ec356f6..628f282 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3115,12 +3115,20 @@ when the shift value is high (how high depends on the host machine). > ETEXI > > DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \ > - "-watchdog i6300esb|ib700\n" \ > - " enable virtual hardware watchdog [default=none]\n", > + "-watchdog [[model=]i6300esb|ib700]\n" \ > + " [,action=reset|shutdown|poweroff|pause|debug|none]\n" \ > + " enable virtual hardware watchdog (default: model=none,action=reset)\n", > + QEMU_ARCH_ALL) > +DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ > + "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ > + " action when watchdog fires (default: reset)\n", > QEMU_ARCH_ALL) > STEXI > -@item -watchdog @var{model} > +@item -watchdog [[model=]@var{model}][,action=@var{action}] > @findex -watchdog > +@itemx -watchdog-action @var{action} > +@findex -watchdog-action > + > Create a virtual hardware watchdog device. Once enabled (by a guest > action), the watchdog must be periodically polled by an agent inside > the guest or else the guest will be restarted. > @@ -3129,22 +3137,16 @@ The @var{model} is the model of hardware watchdog to emulate. Choices > for model are: @code{ib700} (iBASE 700) which is a very simple ISA > watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O > controller hub) which is a much more featureful PCI-based dual-timer > -watchdog. Choose a model for which your guest has drivers. > - > -Use @code{-watchdog help} to list available hardware models. Only one > +watchdog. Choose a model for which your guest has drivers; > +use @code{-watchdog help} to list available hardware models. Only one > watchdog can be enabled for a guest. > -ETEXI > - > -DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ > - "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ > - " action when watchdog fires [default=reset]\n", > - QEMU_ARCH_ALL) > -STEXI > -@item -watchdog-action @var{action} > -@findex -watchdog-action > > The @var{action} controls what QEMU will do when the watchdog timer > -expires. > +expires. It can be specified directly in the @option{-watchdog} option, > +or as the argument of a separate @option{-watchdog-action} parameter. > +The effect is the same. The watchdog action can be specified even if > +the watchdog is enabled through the @option{-device} option. > + > The default is > @code{reset} (forcefully reset the guest). > Other possible actions are: > @@ -3162,8 +3164,9 @@ situations where the watchdog would have expired, and thus > Examples: > > @table @code > -@item -watchdog i6300esb -watchdog-action pause > +@item -watchdog i6300esb,action=pause > @item -watchdog ib700 > +@item -device ib700 -watchdog action=poweroff > @end table > ETEXI > > diff --git a/vl.c b/vl.c > index 9a04ad4..55363fe 100644 > --- a/vl.c > +++ b/vl.c > @@ -168,7 +168,6 @@ static int no_reboot; > int no_shutdown = 0; > int cursor_hide = 1; > int graphic_rotate = 0; > -const char *watchdog; > QEMUOptionRom option_rom[MAX_OPTION_ROMS]; > int nb_option_roms; > int semihosting_enabled = 0; > @@ -475,6 +474,23 @@ static QemuOptsList qemu_icount_opts = { > }, > }; > > +static QemuOptsList qemu_watchdog_opts = { > + .name = "watchdog", > + .implied_opt_name = "model", > + .merge_lists = true, > + .head = QTAILQ_HEAD_INITIALIZER(qemu_watchdog_opts.head), > + .desc = { > + { > + .name = "model", > + .type = QEMU_OPT_STRING, > + }, { > + .name = "action", > + .type = QEMU_OPT_STRING, > + }, > + { /* end of list */ } > + }, > +}; > + > static QemuOptsList qemu_semihosting_config_opts = { > .name = "semihosting-config", > .implied_opt_name = "enable", > @@ -1224,6 +1240,26 @@ static void configure_msg(QemuOpts *opts) > enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true); > } > > +static void configure_watchdog(QemuOpts *opts) > +{ > + const char *s; > + > + s = qemu_opt_get(opts, "model"); > + if (s) { > + int i = select_watchdog(s); > + if (i != 0) { > + exit (i <= 1 ? 1 : 0); > + } > + } > + s = qemu_opt_get(opts, "action"); > + if (s) { > + int i = select_watchdog_action(s); > + if (i != 0) { > + exit (i <= 1 ? 1 : 0); > + } > + } > +} > + > /***********************************************************/ > /* USB devices */ > > @@ -2739,7 +2775,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) > > int main(int argc, char **argv, char **envp) > { > - int i; > int snapshot, linux_boot; > const char *initrd_filename; > const char *kernel_filename, *kernel_cmdline; > @@ -2815,6 +2850,7 @@ int main(int argc, char **argv, char **envp) > qemu_add_opts(&qemu_name_opts); > qemu_add_opts(&qemu_numa_opts); > qemu_add_opts(&qemu_icount_opts); > + qemu_add_opts(&qemu_watchdog_opts); > qemu_add_opts(&qemu_semihosting_config_opts); > > runstate_init(); > @@ -3343,18 +3379,15 @@ int main(int argc, char **argv, char **envp) > } > break; > case QEMU_OPTION_watchdog: > - if (watchdog) { > - fprintf(stderr, > - "qemu: only one watchdog option may be given\n"); > - return 1; > + olist = qemu_find_opts("watchdog"); > + opts = qemu_opts_parse(olist, optarg, 1); > + if (!opts) { > + exit(1); > } > - watchdog = optarg; > break; > case QEMU_OPTION_watchdog_action: > - if (select_watchdog_action(optarg) == -1) { > - fprintf(stderr, "Unknown -watchdog-action parameter\n"); > - exit(1); > - } > + qemu_opts_set(qemu_find_opts("watchdog"), 0, "action", optarg, > + &error_abort); > break; > case QEMU_OPTION_virtiocon: > add_device_config(DEV_VIRTCON, optarg); > @@ -4227,11 +4260,7 @@ int main(int argc, char **argv, char **envp) > select_vgahw(vga_model); > } > > - if (watchdog) { > - i = select_watchdog(watchdog); > - if (i > 0) > - exit (i == 1 ? 1 : 0); > - } > + configure_watchdog(qemu_find_opts_singleton("watchdog")); > > if (machine_class->compat_props) { > qdev_prop_register_global_list(machine_class->compat_props); > -- ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org