Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Eric Blake]

On 01/08/2016 04:27 AM, Denis V. Lunev wrote:

>>>         /* Delete old snapshots of the same name */
>>>       if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) <
>>> 0) {
>>> -        monitor_printf(mon,
>>> -                       "Error while deleting snapshot on device
>>> '%s': %s\n",
>>> -                       bdrv_get_device_name(bs1),
>>> error_get_pretty(local_err));
>>> +        error_setg(errp, "Error while deleting snapshot on device
>>> '%s': %s",
>>> +                   bdrv_get_device_name(bs1),
>>> error_get_pretty(local_err));
>> Markus' series to add a prefixing notation would be better to use here
>> (although I didn't check if he caught this one in that series already):
>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
> 
> this series is not yet merged. I think that we could do this refactoring
> later on.
> This thing could be considered independent. Anyway, this series has its
> own value
> and it takes a lot of time to push it in. Could we do  error setting
> improvement later on?

I don't care who rebases on top of the other, but maybe Markus will have
an opinion when he gets back online next week.


>>> +
>>> +    if (local_err != NULL) {
>> I would have just written 'if (local_err) {'; but that's minor style.
> from my point of view explicit != NULL exposes that local_err is a
> pointer rather than a boolean value.

But the code base already overwhelmingly relies on C's implicit
conversion of pointer to a boolean context, as it requires less typing;
being verbose doesn't make the code base any easier to read.  However,
since HACKING doesn't say one way or the other, I won't make you change.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature