[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 12/27] hmp: hmp_change(): don't use error_get_fi
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 12/27] hmp: hmp_change(): don't use error_get_field() |
Date: |
Wed, 1 Aug 2012 10:51:26 -0300 |
On Wed, 01 Aug 2012 14:39:04 +0200
Markus Armbruster <address@hidden> wrote:
> Luiz Capitulino <address@hidden> 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 <address@hidden>
> > ---
> > 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);
> > }
>