Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging

[Kevin Wolf]

Am 12.07.2011 09:21, schrieb Alexander Graf:
> The monitor command for hotplugging is in i386 specific code. This is just
> plain wrong, as S390 just learned how to do hotplugging too and needs to
> get drives for that.
> 
> So let's add a generic copy to generic code that handles drive_add in a
> way that doesn't have pci dependencies. All pci specific code can then
> be handled in a pci specific function.
> 
> Signed-off-by: Alexander Graf <address@hidden>
> 
> ---
> 
> v1 -> v2:
> 
>   - align generic drive_add to pci specific one
>   - rework to split between generic and pci code
> ---
>  hw/device-hotplug.c |   51 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci-hotplug.c    |   24 ++++--------------------
>  sysemu.h            |    6 +++++-
>  3 files changed, 60 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index 8b2ed7a..eb6dd0e 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -26,6 +26,9 @@
>  #include "boards.h"
>  #include "net.h"
>  #include "blockdev.h"
> +#include "qemu-config.h"
> +#include "sysemu.h"
> +#include "monitor.h"
>  
>  DriveInfo *add_init_drive(const char *optstr)
>  {
> @@ -44,3 +47,51 @@ DriveInfo *add_init_drive(const char *optstr)
>  
>      return dinfo;
>  }
> +
> +#if !defined(TARGET_I386)
> +int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
> +                      DriveInfo *dinfo, int type)
> +{
> +    /* On non-x86 we don't do PCI hotplug */
> +    monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
> +    return -1;
> +}
> +#endif

This assumes that only x86 can do PCI hotplug. If someone added it to
another target, the error should be obvious enough, so I guess it's okay.

> +
> +/*
> + * This version of drive_hot_add does not know anything about PCI specifics.
> + * It is used as fallback on architectures that don't implement pci-hotplug.
> + */

Maybe it was true in v1, don't know, but now it's not a fallback, but a
common implementation that is used by both x86 and s390 and calls a more
specific one in some cases.

It also doesn't make too much sense to have a comment that says what a
function is _not_. Without knowing the context of this patch, I probably
wouldn't understand what the comment is trying to say.

> +void drive_hot_add(Monitor *mon, const QDict *qdict)
> +{
> +    int type;
> +    DriveInfo *dinfo = NULL;
> +    const char *opts = qdict_get_str(qdict, "opts");
> +
> +    dinfo = add_init_drive(opts);
> +    if (!dinfo) {
> +        goto err;
> +    }
> +    if (dinfo->devaddr) {
> +        monitor_printf(mon, "Parameter addr not supported\n");
> +        goto err;
> +    }
> +    type = dinfo->type;
> +
> +    switch (type) {
> +    case IF_NONE:
> +        monitor_printf(mon, "OK\n");
> +        break;
> +    default:
> +        if (pci_drive_hot_add(mon, qdict, dinfo, type)) {
> +            goto err;
> +        }
> +    }
> +    return;
> +
> +err:
> +    if (dinfo) {
> +        drive_put_ref(dinfo);
> +    }
> +    return;
> +}
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index b59be2a..f08d367 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -103,24 +103,13 @@ static int scsi_hot_add(Monitor *mon, DeviceState 
> *adapter,
>      return 0;
>  }
>  
> -void drive_hot_add(Monitor *mon, const QDict *qdict)
> +int pci_drive_hot_add(Monitor *mon, const QDict *qdict,
> +                      DriveInfo *dinfo, int type)
>  {
>      int dom, pci_bus;
>      unsigned slot;
> -    int type;
>      PCIDevice *dev;
> -    DriveInfo *dinfo = NULL;
>      const char *pci_addr = qdict_get_str(qdict, "pci_addr");
> -    const char *opts = qdict_get_str(qdict, "opts");
> -
> -    dinfo = add_init_drive(opts);
> -    if (!dinfo)
> -        goto err;
> -    if (dinfo->devaddr) {
> -        monitor_printf(mon, "Parameter addr not supported\n");
> -        goto err;
> -    }
> -    type = dinfo->type;
>  
>      switch (type) {
>      case IF_SCSI:
> @@ -137,19 +126,14 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
>              goto err;
>          }
>          break;
> -    case IF_NONE:
> -        monitor_printf(mon, "OK\n");
> -        break;
>      default:
>          monitor_printf(mon, "Can't hot-add drive to type %d\n", type);
>          goto err;
>      }
> -    return;
>  
> +    return 0;
>  err:
> -    if (dinfo)
> -        drive_put_ref(dinfo);
> -    return;
> +    return -1;
>  }

Now that there isn't any error handling any more, "goto err;" could be
replaced by "return -1;".

The general approach looks okay.

Kevin