| Message ID | 156871564329.196432.5930574495661947805.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | Fix usage of error_append_hint() | expand |
17.09.2019 13:20, Greg Kurz wrote: > Ensure that hints are added even if errp is &error_fatal or &error_abort. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > block/backup.c | 7 +++++-- > block/dirty-bitmap.c | 7 +++++-- > block/file-posix.c | 20 +++++++++++++------- > block/gluster.c | 23 +++++++++++++++-------- > block/qcow.c | 10 ++++++---- > block/qcow2.c | 7 +++++-- > block/vhdx-log.c | 7 +++++-- > block/vpc.c | 7 +++++-- > 8 files changed, 59 insertions(+), 29 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index 763f0d7ff6db..d8c422a0e3bc 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > BACKUP_CLUSTER_SIZE_DEFAULT); > return BACKUP_CLUSTER_SIZE_DEFAULT; > } else if (ret < 0 && !target->backing) { > - error_setg_errno(errp, -ret, > + Error *local_err = NULL; > + > + error_setg_errno(&local_err, -ret, > "Couldn't determine the cluster size of the target image, " > "which has no backing file"); > - error_append_hint(errp, > + error_append_hint(&local_err, > "Aborting, since this may create an unusable destination image\n"); > + error_propagate(errp, local_err); > return ret; > } else if (ret < 0 && target->backing) { > /* Not fatal; just trudge on ahead. */ Pain.. Do we need these hints, if they are so painful? At least for cases like this, we can create helper function error_setg_errno_hint(..., error, hint) But what could be done when we call function, which may or may not set errp? ret = f(errp); if (ret) { error_append_hint(errp, hint); } Hmmm.. Can it look like ret = f(..., hint_helper(errp, hint)) ? I can't imagine how to do it, as someone should remove hint from error_abort object on success path.. But seems, the following is possible, which seems better for me than local-error approach: error_push_hint(errp, hint); ret = f(.., errp); error_pop_hint(errp); === Continue thinking on this: It may look like just ret = f(..., set_hint(errp, hint)); or (just to split long line): set_hint(errp, hint); ret = f(..., errp); if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit, which may be just one call at function start of macro, which will call error_push_hint(errp) and define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function exit.. Or, may be, more direct way to set cleanup for function exists? === Also, we can implement some code generation, to generate for functions with errp argument wrappers with additional hint parameter, and just use these wrappers.. === If nobody likes any of my suggestions, then ignore them. I understand that this series fixes real issue and much effort has already been spent. May be one day I'll try to rewrite it...
On 9/17/19 5:20 AM, Greg Kurz wrote: > Ensure that hints are added even if errp is &error_fatal or &error_abort. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > block/backup.c | 7 +++++-- > block/dirty-bitmap.c | 7 +++++-- > block/file-posix.c | 20 +++++++++++++------- > block/gluster.c | 23 +++++++++++++++-------- > block/qcow.c | 10 ++++++---- > block/qcow2.c | 7 +++++-- > block/vhdx-log.c | 7 +++++-- > block/vpc.c | 7 +++++-- > 8 files changed, 59 insertions(+), 29 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index 763f0d7ff6db..d8c422a0e3bc 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > BACKUP_CLUSTER_SIZE_DEFAULT); > return BACKUP_CLUSTER_SIZE_DEFAULT; > } else if (ret < 0 && !target->backing) { > - error_setg_errno(errp, -ret, > + Error *local_err = NULL; Can we go with the shorter name 'err' instead of 'local_err'? I know, we aren't consistent (both styles appear throughout the tree), but the shorter style is more appealing to me.
Am 17.09.2019 um 16:39 hat Eric Blake geschrieben: > On 9/17/19 5:20 AM, Greg Kurz wrote: > > Ensure that hints are added even if errp is &error_fatal or &error_abort. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > block/backup.c | 7 +++++-- > > block/dirty-bitmap.c | 7 +++++-- > > block/file-posix.c | 20 +++++++++++++------- > > block/gluster.c | 23 +++++++++++++++-------- > > block/qcow.c | 10 ++++++---- > > block/qcow2.c | 7 +++++-- > > block/vhdx-log.c | 7 +++++-- > > block/vpc.c | 7 +++++-- > > 8 files changed, 59 insertions(+), 29 deletions(-) > > > > diff --git a/block/backup.c b/block/backup.c > > index 763f0d7ff6db..d8c422a0e3bc 100644 > > --- a/block/backup.c > > +++ b/block/backup.c > > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > > BACKUP_CLUSTER_SIZE_DEFAULT); > > return BACKUP_CLUSTER_SIZE_DEFAULT; > > } else if (ret < 0 && !target->backing) { > > - error_setg_errno(errp, -ret, > > + Error *local_err = NULL; > > Can we go with the shorter name 'err' instead of 'local_err'? I know, > we aren't consistent (both styles appear throughout the tree), but the > shorter style is more appealing to me. I like local_err better because it's easier to distinguish from errp. The compiler might catch it if you use the wrong one because one is Error* and the other is Error**, but as a reviewer, I can still get confused. Kevin
On Tue, 17 Sep 2019 13:25:03 +0000 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 17.09.2019 13:20, Greg Kurz wrote: > > Ensure that hints are added even if errp is &error_fatal or &error_abort. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > block/backup.c | 7 +++++-- > > block/dirty-bitmap.c | 7 +++++-- > > block/file-posix.c | 20 +++++++++++++------- > > block/gluster.c | 23 +++++++++++++++-------- > > block/qcow.c | 10 ++++++---- > > block/qcow2.c | 7 +++++-- > > block/vhdx-log.c | 7 +++++-- > > block/vpc.c | 7 +++++-- > > 8 files changed, 59 insertions(+), 29 deletions(-) > > > > diff --git a/block/backup.c b/block/backup.c > > index 763f0d7ff6db..d8c422a0e3bc 100644 > > --- a/block/backup.c > > +++ b/block/backup.c > > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > > BACKUP_CLUSTER_SIZE_DEFAULT); > > return BACKUP_CLUSTER_SIZE_DEFAULT; > > } else if (ret < 0 && !target->backing) { > > - error_setg_errno(errp, -ret, > > + Error *local_err = NULL; > > + > > + error_setg_errno(&local_err, -ret, > > "Couldn't determine the cluster size of the target image, " > > "which has no backing file"); > > - error_append_hint(errp, > > + error_append_hint(&local_err, > > "Aborting, since this may create an unusable destination image\n"); > > + error_propagate(errp, local_err); > > return ret; > > } else if (ret < 0 && target->backing) { > > /* Not fatal; just trudge on ahead. */ > > > Pain.. Do we need these hints, if they are so painful? > I agree that the one above doesn't qualify as a useful hint. It just tells that QEMU is giving up and gives no indication to the user on how to avoid the issue. It should probably be dropped. > At least for cases like this, we can create helper function > > error_setg_errno_hint(..., error, hint) Not very useful if hint has to be forged separately with g_sprintf(), and we can't have such a thing as: error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...) > > But what could be done when we call function, which may or may not set errp? > > ret = f(errp); > if (ret) { > error_append_hint(errp, hint); > } > Same problem. If errp is &error_fatal and f() does errno_setg(errp), it ends up calling exit(). > Hmmm.. > > Can it look like > > ret = f(..., hint_helper(errp, hint)) > > ? > Nope, hint_helper() will get called before f() and things are worse. If errp is NULL then error_append_hint() does nothing and if it is &error_fatal then it aborts. > I can't imagine how to do it, as someone should remove hint from error_abort object on > success path.. > > But seems, the following is possible, which seems better for me than local-error approach: > > error_push_hint(errp, hint); > ret = f(.., errp); > error_pop_hint(errp); > Matter of taste... also, it looks awkward to come up with a hint before knowing what happened. I mean the appropriate hint could depend on the value returned by f() and/or errno for example. > === > > Continue thinking on this: > > It may look like just > ret = f(..., set_hint(errp, hint)); > > or (just to split long line): > set_hint(errp, hint); > ret = f(..., errp); > > if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit, > which may be just one call at function start of macro, which will call error_push_hint(errp) and > define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function > exit.. > > Or, may be, more direct way to set cleanup for function exists? > > === > > Also, we can implement some code generation, to generate for functions with errp argument > wrappers with additional hint parameter, and just use these wrappers.. > > === > > If nobody likes any of my suggestions, then ignore them. I understand that this series fixes > real issue and much effort has already been spent. May be one day I'll try to rewrite it... > For the reason exposed above, I don't think it makes sense to build the hint before calling a function that calls error_setg(). I'm afraid we're stuck with local_err... it is then up to the people to make it as less painful as possible.
On Tue, 17 Sep 2019 16:46:31 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 17.09.2019 um 16:39 hat Eric Blake geschrieben: > > On 9/17/19 5:20 AM, Greg Kurz wrote: > > > Ensure that hints are added even if errp is &error_fatal or &error_abort. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > block/backup.c | 7 +++++-- > > > block/dirty-bitmap.c | 7 +++++-- > > > block/file-posix.c | 20 +++++++++++++------- > > > block/gluster.c | 23 +++++++++++++++-------- > > > block/qcow.c | 10 ++++++---- > > > block/qcow2.c | 7 +++++-- > > > block/vhdx-log.c | 7 +++++-- > > > block/vpc.c | 7 +++++-- > > > 8 files changed, 59 insertions(+), 29 deletions(-) > > > > > > diff --git a/block/backup.c b/block/backup.c > > > index 763f0d7ff6db..d8c422a0e3bc 100644 > > > --- a/block/backup.c > > > +++ b/block/backup.c > > > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > > > BACKUP_CLUSTER_SIZE_DEFAULT); > > > return BACKUP_CLUSTER_SIZE_DEFAULT; > > > } else if (ret < 0 && !target->backing) { > > > - error_setg_errno(errp, -ret, > > > + Error *local_err = NULL; > > > > Can we go with the shorter name 'err' instead of 'local_err'? I know, > > we aren't consistent (both styles appear throughout the tree), but the > > shorter style is more appealing to me. > > I like local_err better because it's easier to distinguish from errp. > The compiler might catch it if you use the wrong one because one is > Error* and the other is Error**, but as a reviewer, I can still get > confused. > I'll favor the official maintainer, hence keeping local_err :) > Kevin
17.09.2019 18:37, Greg Kurz wrote: > On Tue, 17 Sep 2019 13:25:03 +0000 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 17.09.2019 13:20, Greg Kurz wrote: >>> Ensure that hints are added even if errp is &error_fatal or &error_abort. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> --- >>> block/backup.c | 7 +++++-- >>> block/dirty-bitmap.c | 7 +++++-- >>> block/file-posix.c | 20 +++++++++++++------- >>> block/gluster.c | 23 +++++++++++++++-------- >>> block/qcow.c | 10 ++++++---- >>> block/qcow2.c | 7 +++++-- >>> block/vhdx-log.c | 7 +++++-- >>> block/vpc.c | 7 +++++-- >>> 8 files changed, 59 insertions(+), 29 deletions(-) >>> >>> diff --git a/block/backup.c b/block/backup.c >>> index 763f0d7ff6db..d8c422a0e3bc 100644 >>> --- a/block/backup.c >>> +++ b/block/backup.c >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, >>> BACKUP_CLUSTER_SIZE_DEFAULT); >>> return BACKUP_CLUSTER_SIZE_DEFAULT; >>> } else if (ret < 0 && !target->backing) { >>> - error_setg_errno(errp, -ret, >>> + Error *local_err = NULL; >>> + >>> + error_setg_errno(&local_err, -ret, >>> "Couldn't determine the cluster size of the target image, " >>> "which has no backing file"); >>> - error_append_hint(errp, >>> + error_append_hint(&local_err, >>> "Aborting, since this may create an unusable destination image\n"); >>> + error_propagate(errp, local_err); >>> return ret; >>> } else if (ret < 0 && target->backing) { >>> /* Not fatal; just trudge on ahead. */ >> >> >> Pain.. Do we need these hints, if they are so painful? >> > > I agree that the one above doesn't qualify as a useful hint. > It just tells that QEMU is giving up and gives no indication > to the user on how to avoid the issue. It should probably be > dropped. > >> At least for cases like this, we can create helper function >> >> error_setg_errno_hint(..., error, hint) > > Not very useful if hint has to be forged separately with > g_sprintf(), and we can't have such a thing as: > > error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...) > >> >> But what could be done when we call function, which may or may not set errp? >> >> ret = f(errp); >> if (ret) { >> error_append_hint(errp, hint); >> } >> > > Same problem. If errp is &error_fatal and f() does errno_setg(errp), it > ends up calling exit(). > >> Hmmm.. >> >> Can it look like >> >> ret = f(..., hint_helper(errp, hint)) >> >> ? >> > > Nope, hint_helper() will get called before f() and things are worse. > If errp is NULL then error_append_hint() does nothing and if it is > &error_fatal then it aborts. > >> I can't imagine how to do it, as someone should remove hint from error_abort object on >> success path.. >> >> But seems, the following is possible, which seems better for me than local-error approach: >> >> error_push_hint(errp, hint); >> ret = f(.., errp); >> error_pop_hint(errp); >> > > Matter of taste... also, it looks awkward to come up with a hint > before knowing what happened. I mean the appropriate hint could > depend on the value returned by f() and/or errno for example. > >> === >> >> Continue thinking on this: >> >> It may look like just >> ret = f(..., set_hint(errp, hint)); >> >> or (just to split long line): >> set_hint(errp, hint); >> ret = f(..., errp); >> >> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit, >> which may be just one call at function start of macro, which will call error_push_hint(errp) and >> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function >> exit.. >> >> Or, may be, more direct way to set cleanup for function exists? >> >> === >> >> Also, we can implement some code generation, to generate for functions with errp argument >> wrappers with additional hint parameter, and just use these wrappers.. >> >> === >> >> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes >> real issue and much effort has already been spent. May be one day I'll try to rewrite it... >> > > For the reason exposed above, I don't think it makes sense to > build the hint before calling a function that calls error_setg(). > I'm afraid we're stuck with local_err... it is then up to the > people to make it as less painful as possible. > Hmm. so, seems that in general we need local_err.. Then may be, may can make automated propagation? It will look like g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){ .errp = errp, .local_err = NULL, } errp = &_error_prop.local_err; and this thing may be fully covered into macro, to look like this at function start (to be honest it should exactly after all local variable definitions): MAKE_ERRP_SAFE(_error_prop, errp);
On 9/17/19 10:46 AM, Kevin Wolf wrote: > Am 17.09.2019 um 16:39 hat Eric Blake geschrieben: >> On 9/17/19 5:20 AM, Greg Kurz wrote: >>> Ensure that hints are added even if errp is &error_fatal or &error_abort. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> --- >>> block/backup.c | 7 +++++-- >>> block/dirty-bitmap.c | 7 +++++-- >>> block/file-posix.c | 20 +++++++++++++------- >>> block/gluster.c | 23 +++++++++++++++-------- >>> block/qcow.c | 10 ++++++---- >>> block/qcow2.c | 7 +++++-- >>> block/vhdx-log.c | 7 +++++-- >>> block/vpc.c | 7 +++++-- >>> 8 files changed, 59 insertions(+), 29 deletions(-) >>> >>> diff --git a/block/backup.c b/block/backup.c >>> index 763f0d7ff6db..d8c422a0e3bc 100644 >>> --- a/block/backup.c >>> +++ b/block/backup.c >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, >>> BACKUP_CLUSTER_SIZE_DEFAULT); >>> return BACKUP_CLUSTER_SIZE_DEFAULT; >>> } else if (ret < 0 && !target->backing) { >>> - error_setg_errno(errp, -ret, >>> + Error *local_err = NULL; >> >> Can we go with the shorter name 'err' instead of 'local_err'? I know, >> we aren't consistent (both styles appear throughout the tree), but the >> shorter style is more appealing to me. > > I like local_err better because it's easier to distinguish from errp. > The compiler might catch it if you use the wrong one because one is > Error* and the other is Error**, but as a reviewer, I can still get > confused. > > Kevin > Doesn't that sound like a striking reason for condemnation of this entire model? What's going to prevent us from regressing on this technicality in the future?
Am 17.09.2019 um 21:10 hat John Snow geschrieben: > > > On 9/17/19 10:46 AM, Kevin Wolf wrote: > > Am 17.09.2019 um 16:39 hat Eric Blake geschrieben: > >> On 9/17/19 5:20 AM, Greg Kurz wrote: > >>> Ensure that hints are added even if errp is &error_fatal or &error_abort. > >>> > >>> Signed-off-by: Greg Kurz <groug@kaod.org> > >>> --- > >>> block/backup.c | 7 +++++-- > >>> block/dirty-bitmap.c | 7 +++++-- > >>> block/file-posix.c | 20 +++++++++++++------- > >>> block/gluster.c | 23 +++++++++++++++-------- > >>> block/qcow.c | 10 ++++++---- > >>> block/qcow2.c | 7 +++++-- > >>> block/vhdx-log.c | 7 +++++-- > >>> block/vpc.c | 7 +++++-- > >>> 8 files changed, 59 insertions(+), 29 deletions(-) > >>> > >>> diff --git a/block/backup.c b/block/backup.c > >>> index 763f0d7ff6db..d8c422a0e3bc 100644 > >>> --- a/block/backup.c > >>> +++ b/block/backup.c > >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > >>> BACKUP_CLUSTER_SIZE_DEFAULT); > >>> return BACKUP_CLUSTER_SIZE_DEFAULT; > >>> } else if (ret < 0 && !target->backing) { > >>> - error_setg_errno(errp, -ret, > >>> + Error *local_err = NULL; > >> > >> Can we go with the shorter name 'err' instead of 'local_err'? I know, > >> we aren't consistent (both styles appear throughout the tree), but the > >> shorter style is more appealing to me. > > > > I like local_err better because it's easier to distinguish from errp. > > The compiler might catch it if you use the wrong one because one is > > Error* and the other is Error**, but as a reviewer, I can still get > > confused. > > Doesn't that sound like a striking reason for condemnation of this > entire model? Might be, but do you have a better idea? Kevin
On Tue, 17 Sep 2019 17:40:11 +0000 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 17.09.2019 18:37, Greg Kurz wrote: > > On Tue, 17 Sep 2019 13:25:03 +0000 > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > > >> 17.09.2019 13:20, Greg Kurz wrote: > >>> Ensure that hints are added even if errp is &error_fatal or &error_abort. > >>> > >>> Signed-off-by: Greg Kurz <groug@kaod.org> > >>> --- > >>> block/backup.c | 7 +++++-- > >>> block/dirty-bitmap.c | 7 +++++-- > >>> block/file-posix.c | 20 +++++++++++++------- > >>> block/gluster.c | 23 +++++++++++++++-------- > >>> block/qcow.c | 10 ++++++---- > >>> block/qcow2.c | 7 +++++-- > >>> block/vhdx-log.c | 7 +++++-- > >>> block/vpc.c | 7 +++++-- > >>> 8 files changed, 59 insertions(+), 29 deletions(-) > >>> > >>> diff --git a/block/backup.c b/block/backup.c > >>> index 763f0d7ff6db..d8c422a0e3bc 100644 > >>> --- a/block/backup.c > >>> +++ b/block/backup.c > >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, > >>> BACKUP_CLUSTER_SIZE_DEFAULT); > >>> return BACKUP_CLUSTER_SIZE_DEFAULT; > >>> } else if (ret < 0 && !target->backing) { > >>> - error_setg_errno(errp, -ret, > >>> + Error *local_err = NULL; > >>> + > >>> + error_setg_errno(&local_err, -ret, > >>> "Couldn't determine the cluster size of the target image, " > >>> "which has no backing file"); > >>> - error_append_hint(errp, > >>> + error_append_hint(&local_err, > >>> "Aborting, since this may create an unusable destination image\n"); > >>> + error_propagate(errp, local_err); > >>> return ret; > >>> } else if (ret < 0 && target->backing) { > >>> /* Not fatal; just trudge on ahead. */ > >> > >> > >> Pain.. Do we need these hints, if they are so painful? > >> > > > > I agree that the one above doesn't qualify as a useful hint. > > It just tells that QEMU is giving up and gives no indication > > to the user on how to avoid the issue. It should probably be > > dropped. > > > >> At least for cases like this, we can create helper function > >> > >> error_setg_errno_hint(..., error, hint) > > > > Not very useful if hint has to be forged separately with > > g_sprintf(), and we can't have such a thing as: > > > > error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...) > > > >> > >> But what could be done when we call function, which may or may not set errp? > >> > >> ret = f(errp); > >> if (ret) { > >> error_append_hint(errp, hint); > >> } > >> > > > > Same problem. If errp is &error_fatal and f() does errno_setg(errp), it > > ends up calling exit(). > > > >> Hmmm.. > >> > >> Can it look like > >> > >> ret = f(..., hint_helper(errp, hint)) > >> > >> ? > >> > > > > Nope, hint_helper() will get called before f() and things are worse. > > If errp is NULL then error_append_hint() does nothing and if it is > > &error_fatal then it aborts. > > > >> I can't imagine how to do it, as someone should remove hint from error_abort object on > >> success path.. > >> > >> But seems, the following is possible, which seems better for me than local-error approach: > >> > >> error_push_hint(errp, hint); > >> ret = f(.., errp); > >> error_pop_hint(errp); > >> > > > > Matter of taste... also, it looks awkward to come up with a hint > > before knowing what happened. I mean the appropriate hint could > > depend on the value returned by f() and/or errno for example. > > > >> === > >> > >> Continue thinking on this: > >> > >> It may look like just > >> ret = f(..., set_hint(errp, hint)); > >> > >> or (just to split long line): > >> set_hint(errp, hint); > >> ret = f(..., errp); > >> > >> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit, > >> which may be just one call at function start of macro, which will call error_push_hint(errp) and > >> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function > >> exit.. > >> > >> Or, may be, more direct way to set cleanup for function exists? > >> > >> === > >> > >> Also, we can implement some code generation, to generate for functions with errp argument > >> wrappers with additional hint parameter, and just use these wrappers.. > >> > >> === > >> > >> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes > >> real issue and much effort has already been spent. May be one day I'll try to rewrite it... > >> > > > > For the reason exposed above, I don't think it makes sense to > > build the hint before calling a function that calls error_setg(). > > I'm afraid we're stuck with local_err... it is then up to the > > people to make it as less painful as possible. > > > > Hmm. so, seems that in general we need local_err.. > > Then may be, may can make automated propagation? > > It will look like > > g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){ > .errp = errp, > .local_err = NULL, > } > > errp = &_error_prop.local_err; > > and this thing may be fully covered into macro, > to look like this at function start (to be honest it should exactly after all > local variable definitions): > > MAKE_ERRP_SAFE(_error_prop, errp); > > Maybe you can send an RFC patch that converts a handful of local_err users to g_auto() ?
18.09.2019 10:58, Greg Kurz wrote: > On Tue, 17 Sep 2019 17:40:11 +0000 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 17.09.2019 18:37, Greg Kurz wrote: >>> On Tue, 17 Sep 2019 13:25:03 +0000 >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >>> >>>> 17.09.2019 13:20, Greg Kurz wrote: >>>>> Ensure that hints are added even if errp is &error_fatal or &error_abort. >>>>> >>>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>>> --- >>>>> block/backup.c | 7 +++++-- >>>>> block/dirty-bitmap.c | 7 +++++-- >>>>> block/file-posix.c | 20 +++++++++++++------- >>>>> block/gluster.c | 23 +++++++++++++++-------- >>>>> block/qcow.c | 10 ++++++---- >>>>> block/qcow2.c | 7 +++++-- >>>>> block/vhdx-log.c | 7 +++++-- >>>>> block/vpc.c | 7 +++++-- >>>>> 8 files changed, 59 insertions(+), 29 deletions(-) >>>>> >>>>> diff --git a/block/backup.c b/block/backup.c >>>>> index 763f0d7ff6db..d8c422a0e3bc 100644 >>>>> --- a/block/backup.c >>>>> +++ b/block/backup.c >>>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, >>>>> BACKUP_CLUSTER_SIZE_DEFAULT); >>>>> return BACKUP_CLUSTER_SIZE_DEFAULT; >>>>> } else if (ret < 0 && !target->backing) { >>>>> - error_setg_errno(errp, -ret, >>>>> + Error *local_err = NULL; >>>>> + >>>>> + error_setg_errno(&local_err, -ret, >>>>> "Couldn't determine the cluster size of the target image, " >>>>> "which has no backing file"); >>>>> - error_append_hint(errp, >>>>> + error_append_hint(&local_err, >>>>> "Aborting, since this may create an unusable destination image\n"); >>>>> + error_propagate(errp, local_err); >>>>> return ret; >>>>> } else if (ret < 0 && target->backing) { >>>>> /* Not fatal; just trudge on ahead. */ >>>> >>>> >>>> Pain.. Do we need these hints, if they are so painful? >>>> >>> >>> I agree that the one above doesn't qualify as a useful hint. >>> It just tells that QEMU is giving up and gives no indication >>> to the user on how to avoid the issue. It should probably be >>> dropped. >>> >>>> At least for cases like this, we can create helper function >>>> >>>> error_setg_errno_hint(..., error, hint) >>> >>> Not very useful if hint has to be forged separately with >>> g_sprintf(), and we can't have such a thing as: >>> >>> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...) >>> >>>> >>>> But what could be done when we call function, which may or may not set errp? >>>> >>>> ret = f(errp); >>>> if (ret) { >>>> error_append_hint(errp, hint); >>>> } >>>> >>> >>> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it >>> ends up calling exit(). >>> >>>> Hmmm.. >>>> >>>> Can it look like >>>> >>>> ret = f(..., hint_helper(errp, hint)) >>>> >>>> ? >>>> >>> >>> Nope, hint_helper() will get called before f() and things are worse. >>> If errp is NULL then error_append_hint() does nothing and if it is >>> &error_fatal then it aborts. >>> >>>> I can't imagine how to do it, as someone should remove hint from error_abort object on >>>> success path.. >>>> >>>> But seems, the following is possible, which seems better for me than local-error approach: >>>> >>>> error_push_hint(errp, hint); >>>> ret = f(.., errp); >>>> error_pop_hint(errp); >>>> >>> >>> Matter of taste... also, it looks awkward to come up with a hint >>> before knowing what happened. I mean the appropriate hint could >>> depend on the value returned by f() and/or errno for example. >>> >>>> === >>>> >>>> Continue thinking on this: >>>> >>>> It may look like just >>>> ret = f(..., set_hint(errp, hint)); >>>> >>>> or (just to split long line): >>>> set_hint(errp, hint); >>>> ret = f(..., errp); >>>> >>>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit, >>>> which may be just one call at function start of macro, which will call error_push_hint(errp) and >>>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function >>>> exit.. >>>> >>>> Or, may be, more direct way to set cleanup for function exists? >>>> >>>> === >>>> >>>> Also, we can implement some code generation, to generate for functions with errp argument >>>> wrappers with additional hint parameter, and just use these wrappers.. >>>> >>>> === >>>> >>>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes >>>> real issue and much effort has already been spent. May be one day I'll try to rewrite it... >>>> >>> >>> For the reason exposed above, I don't think it makes sense to >>> build the hint before calling a function that calls error_setg(). >>> I'm afraid we're stuck with local_err... it is then up to the >>> people to make it as less painful as possible. >>> >> >> Hmm. so, seems that in general we need local_err.. >> >> Then may be, may can make automated propagation? >> >> It will look like >> >> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){ >> .errp = errp, >> .local_err = NULL, >> } >> >> errp = &_error_prop.local_err; >> >> and this thing may be fully covered into macro, >> to look like this at function start (to be honest it should exactly after all >> local variable definitions): >> >> MAKE_ERRP_SAFE(_error_prop, errp); >> >> > > Maybe you can send an RFC patch that converts a handful of > local_err users to g_auto() ? > Yes, I'll try
diff --git a/block/backup.c b/block/backup.c index 763f0d7ff6db..d8c422a0e3bc 100644 --- a/block/backup.c +++ b/block/backup.c @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, BACKUP_CLUSTER_SIZE_DEFAULT); return BACKUP_CLUSTER_SIZE_DEFAULT; } else if (ret < 0 && !target->backing) { - error_setg_errno(errp, -ret, + Error *local_err = NULL; + + error_setg_errno(&local_err, -ret, "Couldn't determine the cluster size of the target image, " "which has no backing file"); - error_append_hint(errp, + error_append_hint(&local_err, "Aborting, since this may create an unusable destination image\n"); + error_propagate(errp, local_err); return ret; } else if (ret < 0 && target->backing) { /* Not fatal; just trudge on ahead. */ diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 134e0c9a0c8f..b31017a77d51 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -251,10 +251,13 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, if ((flags & BDRV_BITMAP_INCONSISTENT) && bdrv_dirty_bitmap_inconsistent(bitmap)) { - error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used", + Error *local_err = NULL; + + error_setg(&local_err, "Bitmap '%s' is inconsistent and cannot be used", bitmap->name); - error_append_hint(errp, "Try block-dirty-bitmap-remove to delete" + error_append_hint(&local_err, "Try block-dirty-bitmap-remove to delete" " this bitmap from disk"); + error_propagate(errp, local_err); return -1; } diff --git a/block/file-posix.c b/block/file-posix.c index f12c06de2df5..9a95f7e94123 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -389,8 +389,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) } if (!s->buf_align || !bs->bl.request_alignment) { - error_setg(errp, "Could not find working O_DIRECT alignment"); - error_append_hint(errp, "Try cache.direct=off\n"); + Error *local_err = NULL; + + error_setg(&local_err, "Could not find working O_DIRECT alignment"); + error_append_hint(&local_err, "Try cache.direct=off\n"); + error_propagate(errp, local_err); } } @@ -845,16 +848,18 @@ static int raw_handle_perm_lock(BlockDriverState *bs, } ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm, ~s->shared_perm | ~new_shared, - false, errp); + false, &local_err); if (!ret) { - ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp); + ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, &local_err); if (!ret) { return 0; } - error_append_hint(errp, + error_append_hint(&local_err, "Is another process using the image [%s]?\n", bs->filename); } + error_propagate(errp, local_err); + local_err = NULL; /* for fall through */ op = RAW_PL_ABORT; /* fall through to unlock bytes. */ case RAW_PL_ABORT: @@ -2277,11 +2282,12 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp) } /* Step two: Check that nobody else has taken conflicting locks */ - result = raw_check_lock_bytes(fd, perm, shared, errp); + result = raw_check_lock_bytes(fd, perm, shared, &local_err); if (result < 0) { - error_append_hint(errp, + error_append_hint(&local_err, "Is another process using the image [%s]?\n", file_opts->filename); + error_propagate(errp, local_err); goto out_unlock; } diff --git a/block/gluster.c b/block/gluster.c index 64028b2cba30..dce42884f97c 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -473,26 +473,29 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, ret = glfs_init(glfs); if (ret) { - error_setg(errp, "Gluster connection for volume %s, path %s failed" + Error *local_err = NULL; + + error_setg(&local_err, "Gluster connection for volume %s, path %s failed" " to connect", gconf->volume, gconf->path); for (server = gconf->server; server; server = server->next) { if (server->value->type == SOCKET_ADDRESS_TYPE_UNIX) { - error_append_hint(errp, "hint: failed on socket %s ", + error_append_hint(&local_err, "hint: failed on socket %s ", server->value->u.q_unix.path); } else { - error_append_hint(errp, "hint: failed on host %s and port %s ", + error_append_hint(&local_err, "hint: failed on host %s and port %s ", server->value->u.inet.host, server->value->u.inet.port); } } - error_append_hint(errp, "Please refer to gluster logs for more info\n"); + error_append_hint(&local_err, "Please refer to gluster logs for more info\n"); /* glfs_init sometimes doesn't set errno although docs suggest that */ if (errno == 0) { errno = EINVAL; } + error_propagate(errp, local_err); goto out; } return glfs; @@ -695,20 +698,23 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, QDict *options, Error **errp) { int ret; + Error *local_err = NULL; + if (filename) { ret = qemu_gluster_parse_uri(gconf, filename); if (ret < 0) { - error_setg(errp, "invalid URI %s", filename); - error_append_hint(errp, "Usage: file=gluster[+transport]://" + error_setg(&local_err, "invalid URI %s", filename); + error_append_hint(&local_err, "Usage: file=gluster[+transport]://" "[host[:port]]volume/path[?socket=...]" "[,file.debug=N]" "[,file.logfile=/path/filename.log]\n"); + error_propagate(errp, local_err); return ret; } } else { - ret = qemu_gluster_parse_json(gconf, options, errp); + ret = qemu_gluster_parse_json(gconf, options, &local_err); if (ret < 0) { - error_append_hint(errp, "Usage: " + error_append_hint(&local_err, "Usage: " "-drive driver=qcow2,file.driver=gluster," "file.volume=testvol,file.path=/path/a.qcow2" "[,file.debug=9]" @@ -719,6 +725,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, "file.server.1.transport=unix," "file.server.1.path=/var/run/glusterd.socket ..." "\n"); + error_propagate(errp, local_err); return ret; } } diff --git a/block/qcow.c b/block/qcow.c index 5bdf72ba33ce..9049573b52b2 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -156,11 +156,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } if (header.version != QCOW_VERSION) { - error_setg(errp, "qcow (v%d) does not support qcow version %" PRIu32, + error_setg(&local_err, "qcow (v%d) does not support qcow version %" PRIu32, QCOW_VERSION, header.version); if (header.version == 2 || header.version == 3) { - error_append_hint(errp, "Try the 'qcow2' driver instead.\n"); + error_append_hint(&local_err, "Try the 'qcow2' driver instead.\n"); } + error_propagate(errp, local_err); ret = -ENOTSUP; goto fail; @@ -189,14 +190,15 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, if (s->crypt_method_header) { if (bdrv_uses_whitelist() && s->crypt_method_header == QCOW_CRYPT_AES) { - error_setg(errp, + error_setg(&local_err, "Use of AES-CBC encrypted qcow images is no longer " "supported in system emulators"); - error_append_hint(errp, + error_append_hint(&local_err, "You can use 'qemu-img convert' to convert your " "image to an alternative supported format, such " "as unencrypted qcow, or raw with the LUKS " "format instead.\n"); + error_propagate(errp, local_err); ret = -ENOSYS; goto fail; } diff --git a/block/qcow2.c b/block/qcow2.c index 57734f20cf8d..2c52139b7385 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1357,14 +1357,17 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, if (s->crypt_method_header) { if (bdrv_uses_whitelist() && s->crypt_method_header == QCOW_CRYPT_AES) { - error_setg(errp, + Error *local_err = NULL; + + error_setg(&local_err, "Use of AES-CBC encrypted qcow2 images is no longer " "supported in system emulators"); - error_append_hint(errp, + error_append_hint(&local_err, "You can use 'qemu-img convert' to convert your " "image to an alternative supported format, such " "as unencrypted qcow2, or raw with the LUKS " "format instead.\n"); + error_propagate(errp, local_err); ret = -ENOSYS; goto fail; } diff --git a/block/vhdx-log.c b/block/vhdx-log.c index fdd3a7adc378..9f4de9cbcdb6 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -802,15 +802,18 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, if (logs.valid) { if (bs->read_only) { + Error *local_err = NULL; + bdrv_refresh_filename(bs); ret = -EPERM; - error_setg(errp, + error_setg(&local_err, "VHDX image file '%s' opened read-only, but " "contains a log that needs to be replayed", bs->filename); - error_append_hint(errp, "To replay the log, run:\n" + error_append_hint(&local_err, "To replay the log, run:\n" "qemu-img check -r all '%s'\n", bs->filename); + error_propagate(errp, local_err); goto exit; } /* now flush the log */ diff --git a/block/vpc.c b/block/vpc.c index 5cd38907808b..facd7cdb2c98 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -1028,12 +1028,15 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, } if (total_size != total_sectors * BDRV_SECTOR_SIZE) { - error_setg(errp, "The requested image size cannot be represented in " + Error *local_err = NULL; + + error_setg(&local_err, "The requested image size cannot be represented in " "CHS geometry"); - error_append_hint(errp, "Try size=%llu or force-size=on (the " + error_append_hint(&local_err, "Try size=%llu or force-size=on (the " "latter makes the image incompatible with " "Virtual PC)", total_sectors * BDRV_SECTOR_SIZE); + error_propagate(errp, local_err); ret = -EINVAL; goto out; }
Ensure that hints are added even if errp is &error_fatal or &error_abort. Signed-off-by: Greg Kurz <groug@kaod.org> --- block/backup.c | 7 +++++-- block/dirty-bitmap.c | 7 +++++-- block/file-posix.c | 20 +++++++++++++------- block/gluster.c | 23 +++++++++++++++-------- block/qcow.c | 10 ++++++---- block/qcow2.c | 7 +++++-- block/vhdx-log.c | 7 +++++-- block/vpc.c | 7 +++++-- 8 files changed, 59 insertions(+), 29 deletions(-)