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 == _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
> >> _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, >fst, _err);
> >> -if (local_err) {
> >> -error_propagate_prepend(errp, local_err,
> >> +fsdev_throttle_parse_opts(opts, >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 == _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
>> _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, >fst, _err);
>> -if (local_err) {
>> -error_propagate_prepend(errp, local_err,
>> +fsdev_throttle_parse_opts(opts, >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 == _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
>_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, >fst, _err);
> -if (local_err) {
> -error_propagate_prepend(errp, local_err,
> +fsdev_throttle_parse_opts(opts, >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 == _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
_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, >fst, _err);
-if (local_err) {
-error_propagate_prepend(errp, local_err,
+fsdev_throttle_parse_opts(opts, >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
