| Message ID | 20180626135035.133432-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix persistent bitmaps migration logic | expand |
On 06/26/2018 08:50 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 945132f692..46194a33ca 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2114,9 +2114,9 @@ static int qcow2_inactivate(BlockDriverState *bs) > qcow2_store_persistent_dirty_bitmaps(bs, &local_err); > if (local_err != NULL) { > result = -EINVAL; > - error_report_err(local_err); > - error_report("Persistent bitmaps are lost for node '%s'", > - bdrv_get_device_or_node_name(bs)); > + error_reportf_err(local_err, "Persistent bitmaps are lost for node " > + "'%s', because failed to store them on qcow2 " > + "inactivation: ", bdrv_get_device_or_node_name(bs)); That's longer, and has awkward grammar. Also, for a patch designed to improve an error message, it's nice if the commit message demonstrates a before-and-after comparison of the two different wordings, along with a formula for reproducing the error (not mandatory, but it can sure help in reviewing). Shorter might be: "Lost persistent bitmaps during inactivation of node '%s': " since the local_err text appended after the colon will then make it obvious what the error was during inactivation.
On 06/28/2018 08:16 AM, Eric Blake wrote: > On 06/26/2018 08:50 AM, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 945132f692..46194a33ca 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2114,9 +2114,9 @@ static int qcow2_inactivate(BlockDriverState *bs) >> qcow2_store_persistent_dirty_bitmaps(bs, &local_err); >> if (local_err != NULL) { >> result = -EINVAL; >> - error_report_err(local_err); >> - error_report("Persistent bitmaps are lost for node '%s'", >> - bdrv_get_device_or_node_name(bs)); >> + error_reportf_err(local_err, "Persistent bitmaps are lost for >> node " >> + "'%s', because failed to store them on qcow2 " >> + "inactivation: ", >> bdrv_get_device_or_node_name(bs)); > > That's longer, and has awkward grammar. > > Also, for a patch designed to improve an error message, it's nice if the > commit message demonstrates a before-and-after comparison of the two > different wordings, along with a formula for reproducing the error (not > mandatory, but it can sure help in reviewing). > > Shorter might be: > > "Lost persistent bitmaps during inactivation of node '%s': " > > since the local_err text appended after the colon will then make it > obvious what the error was during inactivation. > I like Eric's phrasing suggestion. If you haven't actually managed to trigger this error in real life, I won't demand you artificially do so for the sake of demonstration. --js
diff --git a/block/qcow2.c b/block/qcow2.c index 945132f692..46194a33ca 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2114,9 +2114,9 @@ static int qcow2_inactivate(BlockDriverState *bs) qcow2_store_persistent_dirty_bitmaps(bs, &local_err); if (local_err != NULL) { result = -EINVAL; - error_report_err(local_err); - error_report("Persistent bitmaps are lost for node '%s'", - bdrv_get_device_or_node_name(bs)); + error_reportf_err(local_err, "Persistent bitmaps are lost for node " + "'%s', because failed to store them on qcow2 " + "inactivation: ", bdrv_get_device_or_node_name(bs)); } ret = qcow2_cache_flush(bs, s->l2_table_cache);
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)