qemu-devel
[Top][All Lists]
Advanced

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

[Qemu-devel] Re: [PATCH] Hardware watchdog patch, version 6


From: Anthony Liguori
Subject: [Qemu-devel] Re: [PATCH] Hardware watchdog patch, version 6
Date: Wed, 01 Apr 2009 08:18:24 -0500
User-agent: Thunderbird 2.0.0.21 (X11/20090320)

Richard W.M. Jones wrote:
On Tue, Mar 31, 2009 at 07:07:39PM +0300, Blue Swirl wrote:
+                if (i > 0) exit (i == 1 ? 1 : 0);
+    if (!d->enabled) return;

Please split these into two lines.

Version 7 of the patch is attached:

 - Fixed the 'if' statements above, plus a couple more I found.

 - Rebased to latest SVN.

Rich.

I swear I sent a note with some review comments but I cannot find it in the mailing list archives. Perhaps my mail server ate it :-( I'll reproduce below.

First, there's still no Signed-off-by.

@@ -4727,6 +4732,17 @@
                 serial_devices[serial_device_index] = optarg;
                 serial_device_index++;
                 break;
+            case QEMU_OPTION_watchdog:
+                i = select_watchdog (optarg);

You have extra spaces before all your punctuation. This is not how QEMU does it (just look below).

+                if (i > 0)
+                    exit (i == 1 ? 1 : 0);
+                break;
+            case QEMU_OPTION_watchdog_action:
+                if (select_watchdog_action (optarg) == -1) {
+                    fprintf (stderr, "Unknown -watchdog-action parameter\n");
+                    exit (1);
+                }
+                break;
             case QEMU_OPTION_virtiocon:
                 if (virtio_console_index >= MAX_VIRTIO_CONSOLES) {
                     fprintf(stderr, "qemu: too many virtio consoles\n");

+void register_watchdogs(void)
+{
+#ifdef TARGET_I386
+    wdt_ib700_init ();
+    wdt_i6300esb_init ();
+#endif
+}

Limiting to target-i386 is not correct. These are ISA and PCI devices so they should be limited based on that property.

Really, you only need to limit it based on !CONFIG_USER_ONLY. It's not a huge problem to add unused devices to other platforms. After the config file stuff is ready, we can move to a more modular model where we can choose which devices are included for any given target.

Regards,

Anthony Liguori




reply via email to

[Prev in Thread] Current Thread [Next in Thread]