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

[Anthony Liguori <anthony@codemonkey.ws>]

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