| Message ID | 20191001155319.8066-22-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: auto propagated local_err | expand |
On Tue, 1 Oct 2019 18:53:09 +0300 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > If we want to add some info to errp (by error_prepend() or > error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. > Otherwise, this info will not be added when errp == &fatal_err > (the program will exit prior to the error_append_hint() or > error_prepend() call). Fix such cases. > Well... this patch doesn't really fix anything because... > This commit (together with its neighbors) was generated by > > git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ > spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ > --in-place $f; done > > and then > > ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > > (auto-msg was a file with this commit message) > > and then by hand, for not maintained changed files: > > git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> > > Still, for backporting it may be more comfortable to use only the first > command and then do one huge commit. > > Reported-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > hw/9pfs/9p-local.c | 1 + > hw/9pfs/9p-proxy.c | 1 + > hw/9pfs/9p.c | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 08e673a79c..fccbf758bd 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx) > > static void error_append_security_model_hint(Error **errp) > { > + ERRP_AUTO_PROPAGATE(); > error_append_hint(errp, "Valid options are: security_model=" > "[passthrough|mapped-xattr|mapped-file|none]\n"); > } This function doesn't need auto propagation in the first place. It is simply a wrapper around error_append_hint(). ERRP_AUTO_PROPAGATE() should go to the caller local_parse_opts(). Also some extra care is needed there to handle part [3.] of the cleanup. I understand this is out of the scope of that series, but I'd rather see all of this fixed in the same patch. > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c > index 57a8c1c808..9291c8efa2 100644 > --- a/hw/9pfs/9p-proxy.c > +++ b/hw/9pfs/9p-proxy.c > @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp) > > static void error_append_socket_sockfd_hint(Error **errp) > { > + ERRP_AUTO_PROPAGATE(); > error_append_hint(errp, "Either specify socket=/some/path where /some/path" > " points to a listening AF_UNIX socket or sock_fd=fd" > " where fd is a file descriptor to a connected AF_UNIX" Same here. ERRP_AUTO_PROPAGATE() should go to proxy_parse_opts(). > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index cce2366219..1df2749e03 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr) > int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t, > Error **errp) > { > + ERRP_AUTO_PROPAGATE(); This is correct since this function calls error_prepend() but I think errp is never &error_fatal or &error_abort on the realize path. Anyway, better safe than sorry. > int i, len; > struct stat stat; > FsDriverEntry *fse; Please drop this patch from your series and I'll do the change once ERRP_AUTO_PROPAGATE() gets merged. Great thanks for your time in finding a clever way to deal with error propagation. :) Cheers, -- Greg
02.10.2019 12:19, Greg Kurz wrote: > On Tue, 1 Oct 2019 18:53:09 +0300 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> If we want to add some info to errp (by error_prepend() or >> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. >> Otherwise, this info will not be added when errp == &fatal_err >> (the program will exit prior to the error_append_hint() or >> error_prepend() call). Fix such cases. >> > > Well... this patch doesn't really fix anything because... I'm sure it fixes. But it also breaks some things you mention below.. > >> This commit (together with its neighbors) was generated by >> >> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ >> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ >> --in-place $f; done >> >> and then >> >> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" >> >> (auto-msg was a file with this commit message) >> >> and then by hand, for not maintained changed files: >> >> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> >> >> Still, for backporting it may be more comfortable to use only the first >> command and then do one huge commit. >> >> Reported-by: Greg Kurz <groug@kaod.org> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> hw/9pfs/9p-local.c | 1 + >> hw/9pfs/9p-proxy.c | 1 + >> hw/9pfs/9p.c | 1 + >> 3 files changed, 3 insertions(+) >> >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> index 08e673a79c..fccbf758bd 100644 >> --- a/hw/9pfs/9p-local.c >> +++ b/hw/9pfs/9p-local.c >> @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx) >> >> static void error_append_security_model_hint(Error **errp) >> { >> + ERRP_AUTO_PROPAGATE(); >> error_append_hint(errp, "Valid options are: security_model=" >> "[passthrough|mapped-xattr|mapped-file|none]\n"); >> } > > This function doesn't need auto propagation in the first place. It is > simply a wrapper around error_append_hint(). ERRP_AUTO_PROPAGATE() > should go to the caller local_parse_opts(). Hmm, that's bad. So, actually it's one more errp IN parameter. > > Also some extra care is needed there to handle part [3.] of the > cleanup. I understand this is out of the scope of that series, > but I'd rather see all of this fixed in the same patch. > >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c >> index 57a8c1c808..9291c8efa2 100644 >> --- a/hw/9pfs/9p-proxy.c >> +++ b/hw/9pfs/9p-proxy.c >> @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp) >> >> static void error_append_socket_sockfd_hint(Error **errp) >> { >> + ERRP_AUTO_PROPAGATE(); >> error_append_hint(errp, "Either specify socket=/some/path where /some/path" >> " points to a listening AF_UNIX socket or sock_fd=fd" >> " where fd is a file descriptor to a connected AF_UNIX" > > Same here. ERRP_AUTO_PROPAGATE() should go to proxy_parse_opts(). and this one. > >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >> index cce2366219..1df2749e03 100644 >> --- a/hw/9pfs/9p.c >> +++ b/hw/9pfs/9p.c >> @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr) >> int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t, >> Error **errp) >> { >> + ERRP_AUTO_PROPAGATE(); > > This is correct since this function calls error_prepend() but I think > errp is never &error_fatal or &error_abort on the realize path. Anyway, > better safe than sorry. > >> int i, len; >> struct stat stat; >> FsDriverEntry *fse; > > Please drop this patch from your series and I'll do the change once > ERRP_AUTO_PROPAGATE() gets merged. > > Great thanks for your time in finding a clever way to deal with error > propagation. :) > Hmm, actual question is: how many other errp IN parameters I've broken in these series? I can't simply drop one patch from atomatically genereated series: how people will backport it than, or understand how to backport other crossing things? Actually, for the cases you mean, ERRP_AUTO_PROPAGATE is no-op, as it uses original errp if it's not NULL and don't point to error_fatal. So, nothing is broken here, may be we can proceed as is? But I agree, it looks strange. Eric, what do you think? I tried to change cocci script to @rule0@ // Add invocation to errp-functions ~ identifier fn, fn2; @@ fn(..., Error **errp, ...) { + ERRP_AUTO_PROPAGATE(); <+... + fn2(..., errp, ...) + ... ( error_append_hint(errp, ...); | error_prepend(errp, ...); ) ...+> } but it stubs on hw/vfio/pci.c... and it skips a lot of valid cases, so it's wrong.
02.10.2019 15:58, Vladimir Sementsov-Ogievskiy wrote: > 02.10.2019 12:19, Greg Kurz wrote: >> On Tue, 1 Oct 2019 18:53:09 +0300 >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >>> If we want to add some info to errp (by error_prepend() or >>> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. >>> Otherwise, this info will not be added when errp == &fatal_err >>> (the program will exit prior to the error_append_hint() or >>> error_prepend() call). Fix such cases. >>> >> >> Well... this patch doesn't really fix anything because... > > I'm sure it fixes. But it also breaks some things you mention below.. > >> >>> This commit (together with its neighbors) was generated by >>> >>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ >>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ >>> --in-place $f; done >>> >>> and then >>> >>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" >>> >>> (auto-msg was a file with this commit message) >>> >>> and then by hand, for not maintained changed files: >>> >>> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> >>> >>> Still, for backporting it may be more comfortable to use only the first >>> command and then do one huge commit. >>> >>> Reported-by: Greg Kurz <groug@kaod.org> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> hw/9pfs/9p-local.c | 1 + >>> hw/9pfs/9p-proxy.c | 1 + >>> hw/9pfs/9p.c | 1 + >>> 3 files changed, 3 insertions(+) >>> >>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >>> index 08e673a79c..fccbf758bd 100644 >>> --- a/hw/9pfs/9p-local.c >>> +++ b/hw/9pfs/9p-local.c >>> @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx) >>> static void error_append_security_model_hint(Error **errp) >>> { >>> + ERRP_AUTO_PROPAGATE(); >>> error_append_hint(errp, "Valid options are: security_model=" >>> "[passthrough|mapped-xattr|mapped-file|none]\n"); >>> } >> >> This function doesn't need auto propagation in the first place. It is >> simply a wrapper around error_append_hint(). ERRP_AUTO_PROPAGATE() >> should go to the caller local_parse_opts(). > > Hmm, that's bad. So, actually it's one more errp IN parameter. > >> >> Also some extra care is needed there to handle part [3.] of the >> cleanup. I understand this is out of the scope of that series, >> but I'd rather see all of this fixed in the same patch. >> >>> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c >>> index 57a8c1c808..9291c8efa2 100644 >>> --- a/hw/9pfs/9p-proxy.c >>> +++ b/hw/9pfs/9p-proxy.c >>> @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp) >>> static void error_append_socket_sockfd_hint(Error **errp) >>> { >>> + ERRP_AUTO_PROPAGATE(); >>> error_append_hint(errp, "Either specify socket=/some/path where /some/path" >>> " points to a listening AF_UNIX socket or sock_fd=fd" >>> " where fd is a file descriptor to a connected AF_UNIX" >> >> Same here. ERRP_AUTO_PROPAGATE() should go to proxy_parse_opts(). > > and this one. > >> >>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >>> index cce2366219..1df2749e03 100644 >>> --- a/hw/9pfs/9p.c >>> +++ b/hw/9pfs/9p.c >>> @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr) >>> int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t, >>> Error **errp) >>> { >>> + ERRP_AUTO_PROPAGATE(); >> >> This is correct since this function calls error_prepend() but I think >> errp is never &error_fatal or &error_abort on the realize path. Anyway, >> better safe than sorry. >> >>> int i, len; >>> struct stat stat; >>> FsDriverEntry *fse; >> >> Please drop this patch from your series and I'll do the change once >> ERRP_AUTO_PROPAGATE() gets merged. >> >> Great thanks for your time in finding a clever way to deal with error >> propagation. :) >> > > Hmm, actual question is: how many other errp IN parameters I've broken in > these series? > > I can't simply drop one patch from atomatically genereated series: how people > will backport it than, or understand how to backport other crossing things? > > Actually, for the cases you mean, ERRP_AUTO_PROPAGATE is no-op, as it uses original > errp if it's not NULL and don't point to error_fatal. > > So, nothing is broken here, may be we can proceed as is? But I agree, it looks strange. > > Eric, what do you think? > > I tried to change cocci script to > > @rule0@ > // Add invocation to errp-functions > ~ identifier fn, fn2; > @@ > > fn(..., Error **errp, ...) > { > + ERRP_AUTO_PROPAGATE(); > <+... > + fn2(..., errp, ...) > + ... > ( > error_append_hint(errp, ...); > | > error_prepend(errp, ...); > ) > ...+> > } > > > but it stubs on hw/vfio/pci.c... and it skips a lot of valid cases, so it's wrong. > > > Eric, you are the most interested maintainer, may be, I'll make a v5 with only nbd updated? Then Greg will update his subsystem, then may be I or someone else will update block.. I'm afraid, nobody will review the whole series, and Greg shows, that there maybe some unpredicted effects.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 08e673a79c..fccbf758bd 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx) static void error_append_security_model_hint(Error **errp) { + ERRP_AUTO_PROPAGATE(); error_append_hint(errp, "Valid options are: security_model=" "[passthrough|mapped-xattr|mapped-file|none]\n"); } diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c index 57a8c1c808..9291c8efa2 100644 --- a/hw/9pfs/9p-proxy.c +++ b/hw/9pfs/9p-proxy.c @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp) static void error_append_socket_sockfd_hint(Error **errp) { + ERRP_AUTO_PROPAGATE(); error_append_hint(errp, "Either specify socket=/some/path where /some/path" " points to a listening AF_UNIX socket or sock_fd=fd" " where fd is a file descriptor to a connected AF_UNIX" diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index cce2366219..1df2749e03 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr) int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t, Error **errp) { + ERRP_AUTO_PROPAGATE(); int i, len; struct stat stat; FsDriverEntry *fse;
If we want to add some info to errp (by error_prepend() or error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro. Otherwise, this info will not be added when errp == &fatal_err (the program will exit prior to the error_append_hint() or error_prepend() call). Fix such cases. This commit (together with its neighbors) was generated by git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ --in-place $f; done and then ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" (auto-msg was a file with this commit message) and then by hand, for not maintained changed files: git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> Still, for backporting it may be more comfortable to use only the first command and then do one huge commit. Reported-by: Greg Kurz <groug@kaod.org> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- hw/9pfs/9p-local.c | 1 + hw/9pfs/9p-proxy.c | 1 + hw/9pfs/9p.c | 1 + 3 files changed, 3 insertions(+)