Re: [Qemu-devel] [PATCH 09/17] blockdev: Moving parsing of geometry options to drive_init

["Benoît Canet" <benoit.canet@irqsave.net>]

Le Friday 20 Sep 2013 à 13:54:22 (+0200), Kevin Wolf a écrit :
> This moves all of the geometry options (cyls/heads/secs/trans) to
> drive_init so that they can only be accessed using legacy functions, but
> never with anything blockdev-add related.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 136 +++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 69 insertions(+), 67 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index dc3f01a..ba8b6b4 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -314,7 +314,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>      const char *serial;
>      const char *mediastr = "";
>      int bus_id, unit_id;
> -    int cyls, heads, secs, translation;
>      int max_devs;
>      int index;
>      int ro = 0;
> @@ -332,8 +331,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>      bool has_driver_specific_opts;
>      BlockDriver *drv = NULL;
>  
> -    translation = BIOS_ATA_TRANSLATION_AUTO;
> -
>      /* Check common options by copying from bs_opts to opts, all other options
>       * stay in bs_opts for processing by bdrv_open(). */
>      id = qdict_get_try_str(bs_opts, "id");
> @@ -362,10 +359,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>      unit_id = qemu_opt_get_number(opts, "unit", -1);
>      index   = qemu_opt_get_number(opts, "index", -1);
>  
> -    cyls  = qemu_opt_get_number(opts, "cyls", 0);
> -    heads = qemu_opt_get_number(opts, "heads", 0);
> -    secs  = qemu_opt_get_number(opts, "secs", 0);
> -
>      snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
>      ro = qemu_opt_get_bool(opts, "read-only", 0);
>      copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
> @@ -375,46 +368,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>  
>      max_devs = if_max_devs[type];
>  
> -    if (cyls || heads || secs) {
> -        if (cyls < 1) {
> -            error_report("invalid physical cyls number");
> -	    return NULL;
> -	}
> -        if (heads < 1) {
> -            error_report("invalid physical heads number");
> -	    return NULL;
> -	}
> -        if (secs < 1) {
> -            error_report("invalid physical secs number");
> -	    return NULL;
> -	}
> -    }
> -
> -    if ((buf = qemu_opt_get(opts, "trans")) != NULL) {
> -        if (!cyls) {
> -            error_report("'%s' trans must be used with cyls, heads and secs",
> -                         buf);
> -            return NULL;
> -        }
> -        if (!strcmp(buf, "none"))
> -            translation = BIOS_ATA_TRANSLATION_NONE;
> -        else if (!strcmp(buf, "lba"))
> -            translation = BIOS_ATA_TRANSLATION_LBA;
> -        else if (!strcmp(buf, "auto"))
> -            translation = BIOS_ATA_TRANSLATION_AUTO;
> -	else {
> -            error_report("'%s' invalid translation type", buf);
> -	    return NULL;
> -	}
> -    }
> -
> -    if (media == MEDIA_CDROM) {
> -        if (cyls || secs || heads) {
> -            error_report("CHS can't be set with media=cdrom");
> -            return NULL;
> -        }
> -    }
> -
>      if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
>          if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
>              error_report("invalid discard option");
> @@ -609,10 +562,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
>      dinfo->type = type;
>      dinfo->bus = bus_id;
>      dinfo->unit = unit_id;
> -    dinfo->cyls = cyls;
> -    dinfo->heads = heads;
> -    dinfo->secs = secs;
> -    dinfo->trans = translation;
>      dinfo->refcount = 1;
>      if (serial != NULL) {
>          dinfo->serial = g_strdup(serial);
> @@ -745,6 +694,22 @@ QemuOptsList qemu_legacy_drive_opts = {
>              .name = "if",
>              .type = QEMU_OPT_STRING,
>              .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> +        },{
> +            .name = "cyls",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "number of cylinders (ide disk geometry)",
> +        },{
> +            .name = "heads",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "number of heads (ide disk geometry)",
> +        },{
> +            .name = "secs",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "number of sectors (ide disk geometry)",
> +        },{
> +            .name = "trans",
> +            .type = QEMU_OPT_STRING,
> +            .help = "chs translation (auto, lba. none)",
>          },
>          { /* end of list */ }
>      },
> @@ -758,6 +723,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      QemuOpts *legacy_opts;
>      DriveMediaType media = MEDIA_DISK;
>      BlockInterfaceType type;
> +    int cyls, heads, secs, translation;
>      Error *local_err = NULL;
>  
>      /* Change legacy command line options into QMP ones */
> @@ -847,6 +813,53 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>          type = block_default_type;
>      }
>  
> +    /* Geometry */
> +    cyls  = qemu_opt_get_number(legacy_opts, "cyls", 0);
> +    heads = qemu_opt_get_number(legacy_opts, "heads", 0);
> +    secs  = qemu_opt_get_number(legacy_opts, "secs", 0);
> +
> +    if (cyls || heads || secs) {
> +        if (cyls < 1) {
> +            error_report("invalid physical cyls number");
> +            return NULL;
> +        }
> +        if (heads < 1) {
> +            error_report("invalid physical heads number");
> +            return NULL;
> +        }
> +        if (secs < 1) {
> +            error_report("invalid physical secs number");
> +            return NULL;
> +        }
> +    }
> +
> +    translation = BIOS_ATA_TRANSLATION_AUTO;
> +    value = qemu_opt_get(legacy_opts, "trans");
> +    if (value != NULL) {
> +        if (!cyls) {
> +            error_report("'%s' trans must be used with cyls, heads and secs",
> +                         value);
> +            return NULL;
> +        }
> +        if (!strcmp(value, "none")) {
> +            translation = BIOS_ATA_TRANSLATION_NONE;
> +        } else if (!strcmp(value, "lba")) {
> +            translation = BIOS_ATA_TRANSLATION_LBA;
> +        } else if (!strcmp(value, "auto")) {
> +            translation = BIOS_ATA_TRANSLATION_AUTO;
> +        } else {
> +            error_report("'%s' invalid translation type", value);
> +            return NULL;
> +        }
> +    }
> +
> +    if (media == MEDIA_CDROM) {
> +        if (cyls || secs || heads) {
> +            error_report("CHS can't be set with media=cdrom");
> +            return NULL;
> +        }
> +    }
> +
>      /* Actual block device init: Functionality shared with blockdev-add */
>      dinfo = blockdev_init(bs_opts, type, media);
>      if (dinfo == NULL) {
> @@ -857,6 +870,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      dinfo->enable_auto_del = true;
>      dinfo->opts = all_opts;
>  
> +    dinfo->cyls = cyls;
> +    dinfo->heads = heads;
> +    dinfo->secs = secs;
> +    dinfo->trans = translation;
> +
>  fail:
>      qemu_opts_del(legacy_opts);
>      return dinfo;
> @@ -2200,22 +2218,6 @@ QemuOptsList qemu_common_drive_opts = {
>              .type = QEMU_OPT_NUMBER,
>              .help = "index number",
>          },{
> -            .name = "cyls",
> -            .type = QEMU_OPT_NUMBER,
> -            .help = "number of cylinders (ide disk geometry)",
> -        },{
> -            .name = "heads",
> -            .type = QEMU_OPT_NUMBER,
> -            .help = "number of heads (ide disk geometry)",
> -        },{
> -            .name = "secs",
> -            .type = QEMU_OPT_NUMBER,
> -            .help = "number of sectors (ide disk geometry)",
> -        },{
> -            .name = "trans",
> -            .type = QEMU_OPT_STRING,
> -            .help = "chs translation (auto, lba. none)",
> -        },{
>              .name = "snapshot",
>              .type = QEMU_OPT_BOOL,
>              .help = "enable/disable snapshot mode",
> -- 
> 1.8.1.4
> 
> 

Aside from the "goto fail;" impacting multiple patch:

Reviewed-for-C-bugs-by: Benoit Canet <benoit@irqsave.net>