Re: [RFC v5 054/126] virtio-9p: introduce ERRP_AUTO_PROPAGATE
On Tue, 19 Nov 2019 16:59:23 + Vladimir Sementsov-Ogievskiy wrote: > 19.11.2019 19:56, Greg Kurz wrote: > > Sorry for the late review... > > > > On Fri, 11 Oct 2019 19:04:40 +0300 > > Vladimir Sementsov-Ogievskiy 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 > > > > s/fatal_err/error_fatal/ > > > >> (the program will exit prior to the error_append_hint() or > >> error_prepend() call). Fix such cases. > >> > >> If we want to check error after errp-function call, we need to > >> introduce local_err and than propagate it to errp. Instead, use > > > > s/than/then > > > >> ERRP_AUTO_PROPAGATE macro, benefits are: > >> 1. No need of explicit error_propagate call > >> 2. No need of explicit local_err variable: use errp directly > >> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or > >> &error_fatel, this means that we don't break error_abort > > > > s/error_fatel/error_fatal > > > >> (we'll abort on error_set, not on error_propagate) > >> > >> This commit (together with its neighbors) was generated by > >> > >> for f in $(git grep -l errp \*.[ch]); do \ > >> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ > >> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; > >> \ > >> done; > >> > >> then fix a bit of compilation problems: coccinelle for some reason > >> leaves several > >> f() { > >> ... > >> goto out; > >> ... > >> out: > >> } > >> patterns, with "out:" at function end. > >> > >> then > >> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > >> > >> (auto-msg was a file with this commit message) > >> > >> Still, for backporting it may be more comfortable to use only the first > >> command and then do one huge commit. > >> > >> Reported-by: Kevin Wolf > >> Reported-by: Greg Kurz > >> Signed-off-by: Vladimir Sementsov-Ogievskiy > >> --- > >> hw/9pfs/9p-local.c | 8 > >> hw/9pfs/9p.c | 1 + > >> 2 files changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > >> index 35635e7e7e..aac7989f16 100644 > >> --- a/hw/9pfs/9p-local.c > >> +++ b/hw/9pfs/9p-local.c > >> @@ -1477,9 +1477,9 @@ static void error_append_security_model_hint(Error > >> **errp_in) > >> > >> static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error > >> **errp) > >> { > >> +ERRP_AUTO_PROPAGATE(); > >> const char *sec_model = qemu_opt_get(opts, "security_model"); > >> const char *path = qemu_opt_get(opts, "path"); > >> -Error *local_err = NULL; > >> > >> if (!sec_model) { > >> error_setg(errp, "security_model property not set"); > >> @@ -1507,9 +1507,9 @@ static int local_parse_opts(QemuOpts *opts, > >> FsDriverEntry *fse, Error **errp) > >> return -1; > >> } > >> > >> -fsdev_throttle_parse_opts(opts, &fse->fst, &local_err); > >> -if (local_err) { > >> -error_propagate_prepend(errp, local_err, > >> +fsdev_throttle_parse_opts(opts, &fse->fst, errp); > >> +if (*errp) { > >> +error_prepend(errp, > >> "invalid throttle configuration: "); > > > > The change looks good, apart from the funky indentation. > > > >> return -1; > >> } > >> 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(); > > > > I don't know coccinelle so I'm not sure why ERRP_AUTO_PROPAGATE() was > > added here... but it's definitely not needed in this function. > > > Because this function calls error_prepend() > My bad, sorry for the noise and: Acked-by: Greg Kurz > > > >> int i, len; > >> struct stat stat; > >> FsDriverEntry *fse; > > > >
Re: [RFC v5 054/126] virtio-9p: introduce ERRP_AUTO_PROPAGATE
19.11.2019 19:56, Greg Kurz wrote: > Sorry for the late review... > > On Fri, 11 Oct 2019 19:04:40 +0300 > Vladimir Sementsov-Ogievskiy 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 > > s/fatal_err/error_fatal/ > >> (the program will exit prior to the error_append_hint() or >> error_prepend() call). Fix such cases. >> >> If we want to check error after errp-function call, we need to >> introduce local_err and than propagate it to errp. Instead, use > > s/than/then > >> ERRP_AUTO_PROPAGATE macro, benefits are: >> 1. No need of explicit error_propagate call >> 2. No need of explicit local_err variable: use errp directly >> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or >> &error_fatel, this means that we don't break error_abort > > s/error_fatel/error_fatal > >> (we'll abort on error_set, not on error_propagate) >> >> This commit (together with its neighbors) was generated by >> >> for f in $(git grep -l errp \*.[ch]); do \ >> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ >> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \ >> done; >> >> then fix a bit of compilation problems: coccinelle for some reason >> leaves several >> f() { >> ... >> goto out; >> ... >> out: >> } >> patterns, with "out:" at function end. >> >> then >> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" >> >> (auto-msg was a file with this commit message) >> >> Still, for backporting it may be more comfortable to use only the first >> command and then do one huge commit. >> >> Reported-by: Kevin Wolf >> Reported-by: Greg Kurz >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> hw/9pfs/9p-local.c | 8 >> hw/9pfs/9p.c | 1 + >> 2 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> index 35635e7e7e..aac7989f16 100644 >> --- a/hw/9pfs/9p-local.c >> +++ b/hw/9pfs/9p-local.c >> @@ -1477,9 +1477,9 @@ static void error_append_security_model_hint(Error >> **errp_in) >> >> static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error >> **errp) >> { >> +ERRP_AUTO_PROPAGATE(); >> const char *sec_model = qemu_opt_get(opts, "security_model"); >> const char *path = qemu_opt_get(opts, "path"); >> -Error *local_err = NULL; >> >> if (!sec_model) { >> error_setg(errp, "security_model property not set"); >> @@ -1507,9 +1507,9 @@ static int local_parse_opts(QemuOpts *opts, >> FsDriverEntry *fse, Error **errp) >> return -1; >> } >> >> -fsdev_throttle_parse_opts(opts, &fse->fst, &local_err); >> -if (local_err) { >> -error_propagate_prepend(errp, local_err, >> +fsdev_throttle_parse_opts(opts, &fse->fst, errp); >> +if (*errp) { >> +error_prepend(errp, >> "invalid throttle configuration: "); > > The change looks good, apart from the funky indentation. > >> return -1; >> } >> 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(); > > I don't know coccinelle so I'm not sure why ERRP_AUTO_PROPAGATE() was > added here... but it's definitely not needed in this function. Because this function calls error_prepend() > >> int i, len; >> struct stat stat; >> FsDriverEntry *fse; > -- Best regards, Vladimir
Re: [RFC v5 054/126] virtio-9p: introduce ERRP_AUTO_PROPAGATE
Sorry for the late review... On Fri, 11 Oct 2019 19:04:40 +0300 Vladimir Sementsov-Ogievskiy 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 s/fatal_err/error_fatal/ > (the program will exit prior to the error_append_hint() or > error_prepend() call). Fix such cases. > > If we want to check error after errp-function call, we need to > introduce local_err and than propagate it to errp. Instead, use s/than/then > ERRP_AUTO_PROPAGATE macro, benefits are: > 1. No need of explicit error_propagate call > 2. No need of explicit local_err variable: use errp directly > 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or >&error_fatel, this means that we don't break error_abort s/error_fatel/error_fatal >(we'll abort on error_set, not on error_propagate) > > This commit (together with its neighbors) was generated by > > for f in $(git grep -l errp \*.[ch]); do \ > spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ > --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \ > done; > > then fix a bit of compilation problems: coccinelle for some reason > leaves several > f() { > ... > goto out; > ... > out: > } > patterns, with "out:" at function end. > > then > ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > > (auto-msg was a file with this commit message) > > Still, for backporting it may be more comfortable to use only the first > command and then do one huge commit. > > Reported-by: Kevin Wolf > Reported-by: Greg Kurz > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > hw/9pfs/9p-local.c | 8 > hw/9pfs/9p.c | 1 + > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 35635e7e7e..aac7989f16 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -1477,9 +1477,9 @@ static void error_append_security_model_hint(Error > **errp_in) > > static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp) > { > +ERRP_AUTO_PROPAGATE(); > const char *sec_model = qemu_opt_get(opts, "security_model"); > const char *path = qemu_opt_get(opts, "path"); > -Error *local_err = NULL; > > if (!sec_model) { > error_setg(errp, "security_model property not set"); > @@ -1507,9 +1507,9 @@ static int local_parse_opts(QemuOpts *opts, > FsDriverEntry *fse, Error **errp) > return -1; > } > > -fsdev_throttle_parse_opts(opts, &fse->fst, &local_err); > -if (local_err) { > -error_propagate_prepend(errp, local_err, > +fsdev_throttle_parse_opts(opts, &fse->fst, errp); > +if (*errp) { > +error_prepend(errp, > "invalid throttle configuration: "); The change looks good, apart from the funky indentation. > return -1; > } > 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(); I don't know coccinelle so I'm not sure why ERRP_AUTO_PROPAGATE() was added here... but it's definitely not needed in this function. > int i, len; > struct stat stat; > FsDriverEntry *fse;
[RFC v5 054/126] virtio-9p: introduce ERRP_AUTO_PROPAGATE
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. If we want to check error after errp-function call, we need to introduce local_err and than propagate it to errp. Instead, use ERRP_AUTO_PROPAGATE macro, benefits are: 1. No need of explicit error_propagate call 2. No need of explicit local_err variable: use errp directly 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or &error_fatel, this means that we don't break error_abort (we'll abort on error_set, not on error_propagate) This commit (together with its neighbors) was generated by for f in $(git grep -l errp \*.[ch]); do \ spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \ --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \ done; then fix a bit of compilation problems: coccinelle for some reason leaves several f() { ... goto out; ... out: } patterns, with "out:" at function end. then ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" (auto-msg was a file with this commit message) Still, for backporting it may be more comfortable to use only the first command and then do one huge commit. Reported-by: Kevin Wolf Reported-by: Greg Kurz Signed-off-by: Vladimir Sementsov-Ogievskiy --- hw/9pfs/9p-local.c | 8 hw/9pfs/9p.c | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 35635e7e7e..aac7989f16 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1477,9 +1477,9 @@ static void error_append_security_model_hint(Error **errp_in) static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp) { +ERRP_AUTO_PROPAGATE(); const char *sec_model = qemu_opt_get(opts, "security_model"); const char *path = qemu_opt_get(opts, "path"); -Error *local_err = NULL; if (!sec_model) { error_setg(errp, "security_model property not set"); @@ -1507,9 +1507,9 @@ static int local_parse_opts(QemuOpts *opts, FsDriverEntry *fse, Error **errp) return -1; } -fsdev_throttle_parse_opts(opts, &fse->fst, &local_err); -if (local_err) { -error_propagate_prepend(errp, local_err, +fsdev_throttle_parse_opts(opts, &fse->fst, errp); +if (*errp) { +error_prepend(errp, "invalid throttle configuration: "); return -1; } 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; -- 2.21.0