Re: [Qemu-devel] [PATCH 2/6] blockdev: Allow overriding if_max_dev property

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> The if_max_devs table as in the past been an immutable
> default that controls the mapping of index => (bus,unit)
> for all boards and all HBAs for each interface type.
>
> Since adding this mapping information to the HBA device
> itself is currently unwieldly from the perspective of
> retrieving this information at option parsing time
> (e.g, within drive_new), we consider the alternative
> of marking the if_max_devs table mutable so that
> later configuration and initialization can adjust the
> mapping at will, but only up until a drive is added,
> at which point the mapping is finalized.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c                | 35 ++++++++++++++++++++++++++++++++++-
>  include/sysemu/blockdev.h |  3 +++
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 81398e7..94562e9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -60,7 +60,7 @@ static const char *const if_name[IF_COUNT] = {
>      [IF_XEN] = "xen",
>  };
>  
> -static const int if_max_devs[IF_COUNT] = {
> +static int if_max_devs[IF_COUNT] = {
>      /*
>       * Do not change these numbers!  They govern how drive option
>       * index maps to unit and bus.  That mapping is ABI.
> @@ -79,6 +79,30 @@ static const int if_max_devs[IF_COUNT] = {
>      [IF_SCSI] = 7,
>  };
>  
> +/**
> + * Boards may call this to offer board-by-board overrides
> + * of the default, global values.
> + */
> +void override_max_devs(BlockInterfaceType type, int max_devs)
> +{
> +    DriveInfo *dinfo;
> +
> +    if (max_devs <= 0) {
> +        return;
> +    }
> +
> +    QTAILQ_FOREACH(dinfo, &drives, next) {
> +        if (dinfo->type == type) {
> +            fprintf(stderr, "Warning: Cannot override units-per-bus property of"
> +                    " the %s interface, because a drive of that type has"
> +                    " already been added.\n", if_name[type]);

Isn't this a programming error?  If yes: assert().

> +            return;
> +        }
> +    }
> +
> +    if_max_devs[type] = max_devs;
> +}
> +
>  /*
>   * We automatically delete the drive when a device using it gets
>   * unplugged.  Questionable feature, but we can't just drop it.
> @@ -111,6 +135,15 @@ void blockdev_auto_del(BlockDriverState *bs)
>      }
>  }
>  
> +int if_get_max_devs(BlockInterfaceType type)
> +{
> +    if (type >= IF_IDE && type < IF_COUNT) {
> +        return if_max_devs[type];
> +    }
> +
> +    return 0;
> +}
> +

The need for if_get_max_devs() isn't clear at this point.

>  static int drive_index_to_bus_id(BlockInterfaceType type, int index)
>  {
>      int max_devs = if_max_devs[type];
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 80f768d..10719d5 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -46,10 +46,13 @@ struct DriveInfo {
>      QTAILQ_ENTRY(DriveInfo) next;
>  };
>  
> +void override_max_devs(BlockInterfaceType type, int max_devs);
> +
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>  bool drive_check_orphaned(void);
>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>  int drive_get_max_bus(BlockInterfaceType type);
> +int if_get_max_devs(BlockInterfaceType type);
>  DriveInfo *drive_get_next(BlockInterfaceType type);
>  DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);