Re: [Qemu-devel] [PATCH v2 5/5] bootdevice: add Error **errp argument for QEMUBootSetHandler

[Markus Armbruster]

<address@hidden> writes:

> From: Gonglei <address@hidden>
>
> We can use it for checking when we change traditional
> boot order dynamically and propagate error message
> to the monitor.
> For x86 architecture, we pass &local_err to set_boot_dev()
> when vm startup in pc_coms_init().
>
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Alexander Graf <address@hidden>
> Cc: Blue Swirl <address@hidden>
> Cc: address@hidden
> Signed-off-by: Gonglei <address@hidden>
> ---
>  bootdevice.c            |  5 +----
>  hw/i386/pc.c            | 23 +++++++++++++----------
>  hw/ppc/mac_newworld.c   |  4 ++--
>  hw/ppc/mac_oldworld.c   |  5 ++---
>  hw/sparc/sun4m.c        |  4 ++--
>  hw/sparc64/sun4u.c      |  4 ++--
>  include/sysemu/sysemu.h |  4 ++--
>  7 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/bootdevice.c b/bootdevice.c
> index 9de34ba..5914417 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -63,10 +63,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>          return;
>      }
>  
> -    if (boot_set_handler(boot_set_opaque, boot_order)) {
> -        error_setg(errp, "setting boot device list failed");
> -        return;
> -    }
> +    boot_set_handler(boot_set_opaque, boot_order, errp);
>  }
>  
>  void validate_bootdevices(const char *devices, Error **errp)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c0e55a6..6b7fdea 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -282,7 +282,7 @@ static int boot_device2nibble(char boot_device)
>      return 0;
>  }
>  
> -static int set_boot_dev(ISADevice *s, const char *boot_device)
> +static void set_boot_dev(ISADevice *s, const char *boot_device, Error **errp)
>  {
>  #define PC_MAX_BOOT_DEVICES 3
>      int nbds, bds[3] = { 0, };
> @@ -290,25 +290,24 @@ static int set_boot_dev(ISADevice *s, const char 
> *boot_device)
>  
>      nbds = strlen(boot_device);
>      if (nbds > PC_MAX_BOOT_DEVICES) {
> -        error_report("Too many boot devices for PC");
> -        return(1);
> +        error_setg(errp, "Too many boot devices for PC");
> +        return;
>      }
>      for (i = 0; i < nbds; i++) {
>          bds[i] = boot_device2nibble(boot_device[i]);
>          if (bds[i] == 0) {
> -            error_report("Invalid boot device for PC: '%c'",
> -                         boot_device[i]);
> -            return(1);
> +            error_setg(errp, "Invalid boot device for PC: '%c'",
> +                       boot_device[i]);
> +            return;
>          }
>      }
>      rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
>      rtc_set_memory(s, 0x38, (bds[2] << 4) | (fd_bootchk ? 0x0 : 0x1));
> -    return(0);
>  }
>  
> -static int pc_boot_set(void *opaque, const char *boot_device)
> +static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
>  {
> -    return set_boot_dev(opaque, boot_device);
> +    set_boot_dev(opaque, boot_device, errp);
>  }
>  
>  typedef struct pc_cmos_init_late_arg {
> @@ -365,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
> above_4g_mem_size,
>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>      static pc_cmos_init_late_arg arg;
>      PCMachineState *pc_machine = PC_MACHINE(machine);
> +    Error *local_err = NULL;
>  
>      /* various important CMOS locations needed by PC/Bochs bios */
>  
> @@ -412,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
> above_4g_mem_size,
>      object_property_set_link(OBJECT(machine), OBJECT(s),
>                               "rtc_state", &error_abort);
>  
> -    if (set_boot_dev(s, boot_device)) {
> +    set_boot_dev(s, boot_device, &local_err);
> +    if (local_err) {
> +        error_report("%s", error_get_pretty(local_err));
> +        error_free(local_err);
>          exit(1);
>      }
>  

I wouldn't bother freeing stuff right before exit().  But it's not
wrong.

[...]