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

[Denis V. Lunev]

On 01/08/2016 07:14 PM, Eric Blake wrote:
> 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.
why we have to wait with this set due to this reason?
The code with error_prepend and current code are BOTH
correct. One is a bit shorter then other. Yes, it would
be nice to switch to it, but why this should be done in
this set?

This set solves real problem which has not been addressed
for a long time. Let's proceed, cool and shiny stuff
could be done later on, when it will be merged.

Moreover, merging this set will make my life easier
with merging these changes to our downstream.
Fixes will be merged while improvements will stay
in upstream only.


> > > > +
> > > > +    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.
I do not understand your last words.

I am not agitating you with one approach or another. This
is a reason why I am writing code this way. The code written
this way looks better to me. This code is NEW and this does
not contradict any written rule in coding style policy.

If the code is working and correct, can we just move on with it?

Den