Re: [Qemu-devel] [PATCH 08/19] Convert RTC to use enumerations for configuration parameters

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon,  7 Jun 2010 15:42:21 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> Convert the rtc clock and driftfix parameters to use enums for
> configuration. This ensures strict validation at time of config
> parsing.
> 
> Also fixes a bug in qemu-config.c where 'driftfix' was never
> enabled because this is a target independant compliation unit,
> thus cannot include config-target.h and thus never has the
> TARGET_I386 symbol defined.
> 
> Ideally the 'base' parameter would be an enum too, but this
> value is overloaded to also accept a pretty-printed start
> date.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  hw/mc146818rtc.c |    8 ++++----
>  qemu-config.c    |   27 +++++++++++++++++++++++----
>  sysemu.h         |   20 +++++++++++++++++++-
>  vl.c             |   34 ++++++++++------------------------
>  4 files changed, 56 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index c3e6a70..cbb072c 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -202,7 +202,7 @@ static void rtc_periodic_timer(void *opaque)
>      if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
>          s->cmos_data[RTC_REG_C] |= 0xc0;
>  #ifdef TARGET_I386
> -        if(rtc_td_hack) {
> +        if(rtc_driftfix == QEMU_RTC_DRIFTFIX_SLEW) {
>              if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT)
>                  s->irq_reinject_on_ack_count = 0;		
>              apic_reset_irq_delivered();
> @@ -552,7 +552,7 @@ static int rtc_post_load(void *opaque, int version_id)
>      RTCState *s = opaque;
>  
>      if (version_id >= 2) {
> -        if (rtc_td_hack) {
> +        if (rtc_driftfix == QEMU_RTC_DRIFTFIX_SLEW) {
>              rtc_coalesced_timer_update(s);
>          }
>      }
> @@ -597,7 +597,7 @@ static void rtc_reset(void *opaque)
>      qemu_irq_lower(s->irq);
>  
>  #ifdef TARGET_I386
> -    if (rtc_td_hack)
> +    if (rtc_driftfix == QEMU_RTC_DRIFTFIX_SLEW)
>  	    s->irq_coalesced = 0;
>  #endif
>  }
> @@ -619,7 +619,7 @@ static int rtc_initfn(ISADevice *dev)
>  
>      s->periodic_timer = qemu_new_timer(rtc_clock, rtc_periodic_timer, s);
>  #ifdef TARGET_I386
> -    if (rtc_td_hack)
> +    if (rtc_driftfix == QEMU_RTC_DRIFTFIX_SLEW)
>          s->coalesced_timer =
>              qemu_new_timer(rtc_clock, rtc_coalesced_timer, s);
>  #endif
> diff --git a/qemu-config.c b/qemu-config.c
> index f656e6b..75cddc1 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -288,6 +288,11 @@ QemuOptsList qemu_net_opts = {
>      },
>  };
>  
> +QEMU_ENUM_IMPL(qemu_rtc_clock, QEMU_RTC_CLOCK_LAST,
> +	       "host", "guest");
> +QEMU_ENUM_IMPL(qemu_rtc_driftfix, QEMU_RTC_DRIFTFIX_LAST,
> +	       "none", "slew");
> +
>  QemuOptsList qemu_rtc_opts = {
>      .name = "rtc",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_rtc_opts.head),
> @@ -297,12 +302,26 @@ QemuOptsList qemu_rtc_opts = {
>              .type = QEMU_OPT_STRING,
>          },{
>              .name = "clock",
> -            .type = QEMU_OPT_STRING,
> -#ifdef TARGET_I386
> +            .type = QEMU_OPT_ENUM,
> +            .validate = {
> +                .optEnum = {
> +                    .to_string = qemu_rtc_clock_to_string,
> +                    .to_string_list = qemu_rtc_clock_to_string_list,
> +                    .from_string = qemu_rtc_clock_from_string,
> +                    .last = QEMU_RTC_CLOCK_LAST,
> +                },
> +            },
>          },{
>              .name = "driftfix",
> -            .type = QEMU_OPT_STRING,
> -#endif
> +            .type = QEMU_OPT_ENUM,
> +            .validate = {
> +                .optEnum = {
> +                    .to_string = qemu_rtc_driftfix_to_string,
> +                    .to_string_list = qemu_rtc_driftfix_to_string_list,
> +                    .from_string = qemu_rtc_driftfix_from_string,
> +                    .last = QEMU_RTC_DRIFTFIX_LAST,
> +                },
> +            },
>          },
>          { /* end if list */ }
>      },
> diff --git a/sysemu.h b/sysemu.h
> index f0c5eb8..4d39566 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -120,7 +120,6 @@ extern uint8_t irq0override;
>  extern DisplayType display_type;
>  extern const char *keyboard_layout;
>  extern int win2k_install_hack;
> -extern int rtc_td_hack;
>  extern int alt_grab;
>  extern int ctrl_grab;
>  extern int usb_enabled;
> @@ -133,7 +132,26 @@ extern int no_shutdown;
>  extern int semihosting_enabled;
>  extern int old_param;
>  extern int boot_menu;
> +
>  extern QEMUClock *rtc_clock;
> +extern int rtc_driftfix;
> +
> +enum {
> +    QEMU_RTC_CLOCK_HOST,
> +    QEMU_RTC_CLOCK_GUEST,
> +
> +    QEMU_RTC_CLOCK_LAST
> +};
> +QEMU_ENUM_DECL(qemu_rtc_clock);
> +
> +enum {
> +    QEMU_RTC_DRIFTFIX_NONE,
> +    QEMU_RTC_DRIFTFIX_SLEW,
> +
> +    QEMU_RTC_DRIFTFIX_LAST
> +};
> +QEMU_ENUM_DECL(qemu_rtc_driftfix);
> +
>  
>  #define MAX_NODES 64
>  extern int nb_numa_nodes;
> diff --git a/vl.c b/vl.c
> index 16491c4..0b38d62 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -201,7 +201,7 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>  CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>  CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
>  int win2k_install_hack = 0;
> -int rtc_td_hack = 0;
> +int rtc_driftfix = QEMU_RTC_DRIFTFIX_NONE;
>  int usb_enabled = 0;
>  int singlestep = 0;
>  int smp_cpus = 1;
> @@ -418,6 +418,9 @@ static void configure_rtc_date_offset(const char *startdate, int legacy)
>  static void configure_rtc(QemuOpts *opts)
>  {
>      const char *value;
> +    QEMUClock *clocks[] = { host_clock, vm_clock };
> +    verify_true(ARRAY_SIZE(clocks) == QEMU_RTC_CLOCK_LAST);

 This and the other verify_true() usage are generating the following
build error for me when debug is enabled:

cc1: warnings being treated as errors
/home/lcapitulino/src/qmp-unstable/vl.c: In function ‘configure_rtc’:
/home/lcapitulino/src/qmp-unstable/vl.c:422: error: statement with no effect
/home/lcapitulino/src/qmp-unstable/vl.c: In function ‘drive_init’:
/home/lcapitulino/src/qmp-unstable/vl.c:847: error: statement with no effect
make[1]: *** [vl.o] Error 1
make: *** [subdir-libhw64] Error 2


> +    int clock;
>  
>      value = qemu_opt_get(opts, "base");
>      if (value) {
> @@ -429,28 +432,11 @@ static void configure_rtc(QemuOpts *opts)
>              configure_rtc_date_offset(value, 0);
>          }
>      }
> -    value = qemu_opt_get(opts, "clock");
> -    if (value) {
> -        if (!strcmp(value, "host")) {
> -            rtc_clock = host_clock;
> -        } else if (!strcmp(value, "vm")) {
> -            rtc_clock = vm_clock;
> -        } else {
> -            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
> -            exit(1);
> -        }
> -    }
> -    value = qemu_opt_get(opts, "driftfix");
> -    if (value) {
> -        if (!strcmp(value, "slew")) {
> -            rtc_td_hack = 1;
> -        } else if (!strcmp(value, "none")) {
> -            rtc_td_hack = 0;
> -        } else {
> -            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
> -            exit(1);
> -        }
> -    }
> +
> +    clock = qemu_opt_get_enum(opts, "clock", QEMU_RTC_CLOCK_HOST);
> +    rtc_clock = clocks[clock];
> +
> +    rtc_driftfix = qemu_opt_get_enum(opts, "driftfix", QEMU_RTC_DRIFTFIX_NONE);
>  }
>  
>  /***********************************************************/
> @@ -3149,7 +3135,7 @@ int main(int argc, char **argv, char **envp)
>                  win2k_install_hack = 1;
>                  break;
>              case QEMU_OPTION_rtc_td_hack:
> -                rtc_td_hack = 1;
> +                rtc_driftfix = QEMU_RTC_DRIFTFIX_SLEW;
>                  break;
>              case QEMU_OPTION_acpitable:
>                  do_acpitable_option(optarg);