Re: [Qemu-devel] [PATCH 1/3] blockdev: Rename drive_init(), drive_uninit() to drive_new(), drive_del()

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> The Friday 06 Jun 2014 à 14:50:58 (+0200), Markus Armbruster wrote :
>> "Init" and "uninit" suggest the functions don't allocate / free
>> storage.  But they do.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  blockdev.c                | 14 +++++++-------
>>  device-hotplug.c          |  2 +-
>>  hw/usb/dev-storage.c      |  2 +-
>>  include/sysemu/blockdev.h |  4 ++--
>>  vl.c                      |  4 ++--
>>  5 files changed, 13 insertions(+), 13 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 9b5261b..1256013 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -213,7 +213,7 @@ static void bdrv_format_print(void *opaque, const char 
>> *name)
>>      error_printf(" %s", name);
>>  }
>>  
>> -static void drive_uninit(DriveInfo *dinfo)
>> +static void drive_del(DriveInfo *dinfo)
>>  {
>>      if (dinfo->opts) {
>>          qemu_opts_del(dinfo->opts);
>> @@ -230,7 +230,7 @@ void drive_put_ref(DriveInfo *dinfo)
>>  {
>>      assert(dinfo->refcount);
>>      if (--dinfo->refcount == 0) {
>> -        drive_uninit(dinfo);
>> +        drive_del(dinfo);
>>      }
>>  }
>>  
>> @@ -658,7 +658,7 @@ QemuOptsList qemu_legacy_drive_opts = {
>>      },
>>  };
>>  
>> -DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType 
>> block_default_type)
>> +DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
>> block_default_type)
>>  {
>>      const char *value;
>>      DriveInfo *dinfo = NULL;
>> @@ -1791,7 +1791,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
>> QObject **ret_data)
>>          bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
>>                            BLOCKDEV_ON_ERROR_REPORT);
>>      } else {
>> -        drive_uninit(drive_get_by_blockdev(bs));
>> +        drive_del(drive_get_by_blockdev(bs));
>>      }
>>  
>>      return 0;
>> @@ -2334,9 +2334,9 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
>> **errp)
>>          goto fail;
>>      }
>>  
>> -    /* TODO Sort it out in raw-posix and drive_init: Reject aio=native with
>> +    /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with
>>       * cache.direct=false instead of silently switching to aio=threads, 
>> except
>> -     * if called from drive_init.
>> +     * when called from drive_new().
>>       *
>>       * For now, simply forbidding the combination for all drivers will do. 
>> */
>>      if (options->has_aio && options->aio == BLOCKDEV_AIO_OPTIONS_NATIVE) {
>> @@ -2368,7 +2368,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
>> **errp)
>>      }
>>  
>>      if (bdrv_key_required(dinfo->bdrv)) {
>> -        drive_uninit(dinfo);
>> +        drive_del(dinfo);
>>          error_setg(errp, "blockdev-add doesn't support encrypted devices");
>>          goto fail;
>>      }
>> diff --git a/device-hotplug.c b/device-hotplug.c
>> index eecb08e..fc09d10 100644
>> --- a/device-hotplug.c
>> +++ b/device-hotplug.c
>> @@ -40,7 +40,7 @@ DriveInfo *add_init_drive(const char *optstr)
>>          return NULL;
>>  
>>      mc = MACHINE_GET_CLASS(current_machine);
>> -    dinfo = drive_init(opts, mc->block_default_type);
>> +    dinfo = drive_new(opts, mc->block_default_type);
>>      if (!dinfo) {
>>          qemu_opts_del(opts);
>>          return NULL;
>> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
>> index e919100..ae4efcb 100644
>> --- a/hw/usb/dev-storage.c
>> +++ b/hw/usb/dev-storage.c
>> @@ -691,7 +691,7 @@ static USBDevice *usb_msd_init(USBBus *bus, const char 
>> *filename)
>>      qemu_opt_set(opts, "if", "none");
>>  
>>      /* create host drive */
>> -    dinfo = drive_init(opts, 0);
>> +    dinfo = drive_new(opts, 0);
>>      if (!dinfo) {
>>          qemu_opts_del(opts);
>>          return NULL;
>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>> index 134712b..0fdbd68 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -37,7 +37,7 @@ struct DriveInfo {
>>      int bus;
>>      int unit;
>>      int auto_del;               /* see blockdev_mark_auto_del() */
>> -    bool enable_auto_del; /* Only for legacy drive_init() */
>> +    bool enable_auto_del;       /* Only for legacy drive_new() */
>>      int media_cd;
>>      int cyls, heads, secs, trans;
>>      QemuOpts *opts;
>> @@ -57,7 +57,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
>>  QemuOpts *drive_def(const char *optstr);
>>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>>                      const char *optstr);
>> -DriveInfo *drive_init(QemuOpts *arg, BlockInterfaceType block_default_type);
>> +DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
>>  
>>  /* device-hotplug */
>>  
>> diff --git a/vl.c b/vl.c
> gG> index a3def97..f4b4841 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1074,7 +1074,7 @@ static int drive_init_func(QemuOpts *opts, void 
>> *opaque)
>>  {
>>      BlockInterfaceType *block_default_type = opaque;
>>  
>> -    return drive_init(opts, *block_default_type) == NULL;
>> +    return drive_new(opts, *block_default_type) == NULL;
>>  }
>>  
>>  static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
>> @@ -1098,7 +1098,7 @@ static void default_drive(int enable, int snapshot, 
>> BlockInterfaceType type,
>>      if (snapshot) {
>>          drive_enable_snapshot(opts, NULL);
>>      }
>> -    if (!drive_init(opts, type)) {
>> +    if (!drive_new(opts, type)) {
>
> Unrelated but it seems QEMU is leaking this one on exit.

The kernel cleans it up for free, along with zillions of other unfreed
pieces of memory, file descriptors, and more.

>
>>          exit(1);
>>      }
>>  }
>> -- 
>> 1.9.3
>> 
>> 
>
> Reviewed-by: Benoit Canet <address@hidden>

Thanks!