[12/27] hmp: hmp_change(): don't use error_get_field()

Use the 'device' passed by the user and call qmp_query_block() to
get the 'filename' info.

error_get_field() is going to be dropped by a future commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Use the 'device' passed by the user and call qmp_query_block() to
> get the 'filename' info.
>
> error_get_field() is going to be dropped by a future commit.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hmp.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index a906f8a..435c9cd 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -783,17 +783,35 @@ static void hmp_change_read_arg(Monitor *mon, const char *password,
>  static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
>                                     void *opaque)
>  {
> -    Error *encryption_err = opaque;
> +    char *device = opaque;
>      Error *err = NULL;
> -    const char *device;
> -
> -    device = error_get_field(encryption_err, "device");
>  
>      qmp_block_passwd(device, password, &err);
>      hmp_handle_error(mon, &err);
> -    error_free(encryption_err);
>  
>      monitor_read_command(mon, 1);
> +    g_free(device);
> +}
> +
> +static char *get_device_file(const char *device)
> +{
> +    BlockInfoList *bdev_list, *bdev;
> +    char *ret;
> +
> +    bdev_list = qmp_query_block(NULL);
> +    for (bdev = bdev_list; bdev; bdev = bdev->next) {
> +        if (!strcmp(bdev->value->device, device)) {
> +            break;
> +        }
> +    }
> +
> +    assert(bdev);
> +    assert(bdev->value->has_inserted);
> +
> +    ret = g_strdup(bdev->value->inserted->file);
> +    qapi_free_BlockInfoList(bdev_list);
> +
> +    return ret;
>  }
>  
>  void hmp_change(Monitor *mon, const QDict *qdict)
> @@ -814,9 +832,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>  
>      qmp_change(device, target, !!arg, arg, &err);
>      if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
> -        monitor_printf(mon, "%s (%s) is encrypted.\n",
> -                       error_get_field(err, "device"),
> -                       error_get_field(err, "filename"));
> +        char *filename = get_device_file(device);

Elsewhere, we use bdrv_get_encrypted_filename(), which does the right
thing for encrypted backing files.  Why is that not necessary here?

Why not simply bdrv_get_encrypted_filename(bdrv_find(device))?

> +        monitor_printf(mon, "%s (%s) is encrypted.\n", device, filename);
> +        g_free(filename);
>          if (!monitor_get_rs(mon)) {
>              monitor_printf(mon,
>                      "terminal does not support password prompting\n");
> @@ -824,9 +842,10 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>              return;
>          }
>          readline_start(monitor_get_rs(mon), "Password: ", 1,
> -                       cb_hmp_change_bdrv_pwd, err);
> +                       cb_hmp_change_bdrv_pwd, (void *) g_strdup(device));

Casting a pointer to void * is pointless noise :)

>          return;
>      }
> +
>      hmp_handle_error(mon, &err);
>  }

Markus Armbruster <armbru@redhat.com> writes:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> Use the 'device' passed by the user and call qmp_query_block() to
>> get the 'filename' info.
>>
>> error_get_field() is going to be dropped by a future commit.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  hmp.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index a906f8a..435c9cd 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -783,17 +783,35 @@ static void hmp_change_read_arg(Monitor *mon, const char *password,
>>  static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
>>                                     void *opaque)
>>  {
>> -    Error *encryption_err = opaque;
>> +    char *device = opaque;
>>      Error *err = NULL;
>> -    const char *device;
>> -
>> -    device = error_get_field(encryption_err, "device");
>>  
>>      qmp_block_passwd(device, password, &err);
>>      hmp_handle_error(mon, &err);
>> -    error_free(encryption_err);
>>  
>>      monitor_read_command(mon, 1);
>> +    g_free(device);
>> +}
>> +
>> +static char *get_device_file(const char *device)
>> +{
>> +    BlockInfoList *bdev_list, *bdev;
>> +    char *ret;
>> +
>> +    bdev_list = qmp_query_block(NULL);
>> +    for (bdev = bdev_list; bdev; bdev = bdev->next) {
>> +        if (!strcmp(bdev->value->device, device)) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    assert(bdev);
>> +    assert(bdev->value->has_inserted);
>> +
>> +    ret = g_strdup(bdev->value->inserted->file);
>> +    qapi_free_BlockInfoList(bdev_list);
>> +
>> +    return ret;
>>  }
>>  
>>  void hmp_change(Monitor *mon, const QDict *qdict)
>> @@ -814,9 +832,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>>  
>>      qmp_change(device, target, !!arg, arg, &err);
>>      if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
>> -        monitor_printf(mon, "%s (%s) is encrypted.\n",
>> -                       error_get_field(err, "device"),
>> -                       error_get_field(err, "filename"));
>> +        char *filename = get_device_file(device);
>
> Elsewhere, we use bdrv_get_encrypted_filename(), which does the right
> thing for encrypted backing files.  Why is that not necessary here?
>
> Why not simply bdrv_get_encrypted_filename(bdrv_find(device))?

Those are not QMP functions.  hmp.c is only allowed to use QMP interfaces.

Regards,

Anthony Liguori

On Wed, 01 Aug 2012 14:39:04 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Use the 'device' passed by the user and call qmp_query_block() to
> > get the 'filename' info.
> >
> > error_get_field() is going to be dropped by a future commit.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp.c | 37 ++++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index a906f8a..435c9cd 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -783,17 +783,35 @@ static void hmp_change_read_arg(Monitor *mon, const char *password,
> >  static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
> >                                     void *opaque)
> >  {
> > -    Error *encryption_err = opaque;
> > +    char *device = opaque;
> >      Error *err = NULL;
> > -    const char *device;
> > -
> > -    device = error_get_field(encryption_err, "device");
> >  
> >      qmp_block_passwd(device, password, &err);
> >      hmp_handle_error(mon, &err);
> > -    error_free(encryption_err);
> >  
> >      monitor_read_command(mon, 1);
> > +    g_free(device);
> > +}
> > +
> > +static char *get_device_file(const char *device)
> > +{
> > +    BlockInfoList *bdev_list, *bdev;
> > +    char *ret;
> > +
> > +    bdev_list = qmp_query_block(NULL);
> > +    for (bdev = bdev_list; bdev; bdev = bdev->next) {
> > +        if (!strcmp(bdev->value->device, device)) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    assert(bdev);
> > +    assert(bdev->value->has_inserted);
> > +
> > +    ret = g_strdup(bdev->value->inserted->file);
> > +    qapi_free_BlockInfoList(bdev_list);
> > +
> > +    return ret;
> >  }
> >  
> >  void hmp_change(Monitor *mon, const QDict *qdict)
> > @@ -814,9 +832,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> >  
> >      qmp_change(device, target, !!arg, arg, &err);
> >      if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
> > -        monitor_printf(mon, "%s (%s) is encrypted.\n",
> > -                       error_get_field(err, "device"),
> > -                       error_get_field(err, "filename"));
> > +        char *filename = get_device_file(device);
> 
> Elsewhere, we use bdrv_get_encrypted_filename(), which does the right
> thing for encrypted backing files.  Why is that not necessary here?
> 
> Why not simply bdrv_get_encrypted_filename(bdrv_find(device))?
> 
> > +        monitor_printf(mon, "%s (%s) is encrypted.\n", device, filename);
> > +        g_free(filename);
> >          if (!monitor_get_rs(mon)) {
> >              monitor_printf(mon,
> >                      "terminal does not support password prompting\n");
> > @@ -824,9 +842,10 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> >              return;
> >          }
> >          readline_start(monitor_get_rs(mon), "Password: ", 1,
> > -                       cb_hmp_change_bdrv_pwd, err);
> > +                       cb_hmp_change_bdrv_pwd, (void *) g_strdup(device));
> 
> Casting a pointer to void * is pointless noise :)

I don't remember why I did it, but I've fixed for v1.

Btw, this patch is different v1 because I'm dropping error_is_type()
usage (like I did for the previous patch).

Btw 2, I'm assuming that the other questions you asked have already been
answered in other emails.

> 
> >          return;
> >      }
> > +
> >      hmp_handle_error(mon, &err);
> >  }
>