[v4,02/11] block: add error parameter to del_existing_snapshots()

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 savevm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  savevm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 77c5291..dc1f4a4 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2224,7 +2224,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>  /*
>   * Deletes snapshots of a given name in all opened images.
>   */
> -static int del_existing_snapshots(Monitor *mon, const char *name)
> +static int del_existing_snapshots(const char *name, Error **errp)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
> @@ -2237,9 +2237,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
>              if (ret < 0) {
> -                monitor_printf(mon,
> -                               "Error while deleting snapshot on '%s'\n",
> -                               bdrv_get_device_name(bs));
> +                error_setg(errp, "error while deleting snapshot on '%s'",
> +                           bdrv_get_device_name(bs));
>                  return -1;
>              }
>          }

Any particular reason for de-capitalizing "Error"?

> @@ -2259,6 +2258,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      qemu_timeval tv;
>      struct tm tm;
>      const char *name = qdict_get_try_str(qdict, "name");
> +    Error *local_err = NULL;
>  
>      /* Verify if there is a device that doesn't support snapshots and is writable */
>      bs = NULL;
> @@ -2307,7 +2307,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(mon, name) < 0) {
> +    if (name && del_existing_snapshots(name, &local_err) < 0) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> +        error_free(local_err);
>          goto the_end;
>      }

Could use hmp_handle_error(), except it's static in hmp.c.  On the other
hand, even hmp.c doesn't always use it...  Luiz, what do you think?

On Tue, 09 Apr 2013 15:27:32 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Pavel Hrdina <phrdina@redhat.com> writes:
> 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  savevm.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/savevm.c b/savevm.c
> > index 77c5291..dc1f4a4 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2224,7 +2224,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> >  /*
> >   * Deletes snapshots of a given name in all opened images.
> >   */
> > -static int del_existing_snapshots(Monitor *mon, const char *name)
> > +static int del_existing_snapshots(const char *name, Error **errp)
> >  {
> >      BlockDriverState *bs;
> >      QEMUSnapshotInfo sn1, *snapshot = &sn1;
> > @@ -2237,9 +2237,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> >          {
> >              ret = bdrv_snapshot_delete(bs, name);
> >              if (ret < 0) {
> > -                monitor_printf(mon,
> > -                               "Error while deleting snapshot on '%s'\n",
> > -                               bdrv_get_device_name(bs));
> > +                error_setg(errp, "error while deleting snapshot on '%s'",
> > +                           bdrv_get_device_name(bs));
> >                  return -1;
> >              }
> >          }
> 
> Any particular reason for de-capitalizing "Error"?
> 
> > @@ -2259,6 +2258,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> >      qemu_timeval tv;
> >      struct tm tm;
> >      const char *name = qdict_get_try_str(qdict, "name");
> > +    Error *local_err = NULL;
> >  
> >      /* Verify if there is a device that doesn't support snapshots and is writable */
> >      bs = NULL;
> > @@ -2307,7 +2307,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> >      }
> >  
> >      /* Delete old snapshots of the same name */
> > -    if (name && del_existing_snapshots(mon, name) < 0) {
> > +    if (name && del_existing_snapshots(name, &local_err) < 0) {
> > +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> > +        error_free(local_err);
> >          goto the_end;
> >      }
> 
> Could use hmp_handle_error(), except it's static in hmp.c.  On the other
> hand, even hmp.c doesn't always use it...  Luiz, what do you think?

I've added hmp_handle_error() to avoid copy & paste in hmp.c, but I don't
think it's a good function because it doesn't actually handle the error
and it does two unrelated things (print the error and free it).

So, I think it's better to restrict it to hmp.c.

On 9.4.2013 15:27, Markus Armbruster wrote:
> Pavel Hrdina <phrdina@redhat.com> writes:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   savevm.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 77c5291..dc1f4a4 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2224,7 +2224,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>   /*
>>    * Deletes snapshots of a given name in all opened images.
>>    */
>> -static int del_existing_snapshots(Monitor *mon, const char *name)
>> +static int del_existing_snapshots(const char *name, Error **errp)
>>   {
>>       BlockDriverState *bs;
>>       QEMUSnapshotInfo sn1, *snapshot = &sn1;
>> @@ -2237,9 +2237,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>>           {
>>               ret = bdrv_snapshot_delete(bs, name);
>>               if (ret < 0) {
>> -                monitor_printf(mon,
>> -                               "Error while deleting snapshot on '%s'\n",
>> -                               bdrv_get_device_name(bs));
>> +                error_setg(errp, "error while deleting snapshot on '%s'",
>> +                           bdrv_get_device_name(bs));
>>                   return -1;
>>               }
>>           }
>
> Any particular reason for de-capitalizing "Error"?

Yes, Eric told me that we should start the error message with lover case 
and also we shouldn't print '.' at the end of the error message.

>
>> @@ -2259,6 +2258,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>>       qemu_timeval tv;
>>       struct tm tm;
>>       const char *name = qdict_get_try_str(qdict, "name");
>> +    Error *local_err = NULL;
>>
>>       /* Verify if there is a device that doesn't support snapshots and is writable */
>>       bs = NULL;
>> @@ -2307,7 +2307,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>>       }
>>
>>       /* Delete old snapshots of the same name */
>> -    if (name && del_existing_snapshots(mon, name) < 0) {
>> +    if (name && del_existing_snapshots(name, &local_err) < 0) {
>> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
>> +        error_free(local_err);
>>           goto the_end;
>>       }
>
> Could use hmp_handle_error(), except it's static in hmp.c.  On the other
> hand, even hmp.c doesn't always use it...  Luiz, what do you think?
>

Pavel Hrdina <phrdina@redhat.com> writes:

> On 9.4.2013 15:27, Markus Armbruster wrote:
>> Pavel Hrdina <phrdina@redhat.com> writes:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   savevm.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/savevm.c b/savevm.c
>>> index 77c5291..dc1f4a4 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -2224,7 +2224,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>   /*
>>>    * Deletes snapshots of a given name in all opened images.
>>>    */
>>> -static int del_existing_snapshots(Monitor *mon, const char *name)
>>> +static int del_existing_snapshots(const char *name, Error **errp)
>>>   {
>>>       BlockDriverState *bs;
>>>       QEMUSnapshotInfo sn1, *snapshot = &sn1;
>>> @@ -2237,9 +2237,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>>>           {
>>>               ret = bdrv_snapshot_delete(bs, name);
>>>               if (ret < 0) {
>>> -                monitor_printf(mon,
>>> -                               "Error while deleting snapshot on '%s'\n",
>>> -                               bdrv_get_device_name(bs));
>>> +                error_setg(errp, "error while deleting snapshot on '%s'",
>>> +                           bdrv_get_device_name(bs));
>>>                   return -1;
>>>               }
>>>           }
>>
>> Any particular reason for de-capitalizing "Error"?
>
> Yes, Eric told me that we should start the error message with lover
> case and also we shouldn't print '.' at the end of the error message.

I agree on not ending error messages with a period.  Whether to start
them with a capital letter or not isn't worth arguing over, given the
general state of our error reporting.

[...]

On 04/10/2013 03:57 AM, Pavel Hrdina wrote:

>> Any particular reason for de-capitalizing "Error"?
> 
> Yes, Eric told me that we should start the error message with lover case
> and also we shouldn't print '.' at the end of the error message.

More precisely, I pointed to 'git grep' results that proved that the
code base overwhelmingly avoids trailing '.', but that it was rather
ambiguous on whether messages start with a capital or a lower case.
Changing the period was important, changing the capitalization feels a
bit like churn unless we set a coding standard in HACKING telling which
way new code should use.  I can live with the churn in this commit as
long as you are changing other things in the same hunk at the same time,
but I wasn't specifically asking for capitalization changes.