| Message ID | 1392138233-26407-10-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/cow.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/block/cow.c b/block/cow.c > index 7fc0b12..43a2150 100644 > --- a/block/cow.c > +++ b/block/cow.c > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, > char version[64]; > snprintf(version, sizeof(version), > "COW version %d", cow_header.version); > - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > bs->device_name, "cow", version); > ret = -ENOTSUP; > goto fail; > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > struct stat st; > int64_t image_sectors = 0; > const char *image_filename = NULL; > - Error *local_err = NULL; > int ret; > BlockDriverState *cow_bs; > > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > options++; > } > > - ret = bdrv_create_file(filename, options, &local_err); > + ret = bdrv_create_file(filename, options, errp); > if (ret < 0) { > - qerror_report_err(local_err); > - error_free(local_err); > return ret; > } > > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, > - &local_err); > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); > if (ret < 0) { > - qerror_report_err(local_err); > - error_free(local_err); > return ret; > } This is technically correct, but I think general policy is that using the local_err pattern is preferred anyway. Kevin
On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote: > Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben: > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > block/cow.c | 12 +++--------- > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > diff --git a/block/cow.c b/block/cow.c > > index 7fc0b12..43a2150 100644 > > --- a/block/cow.c > > +++ b/block/cow.c > > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, > > char version[64]; > > snprintf(version, sizeof(version), > > "COW version %d", cow_header.version); > > - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > > bs->device_name, "cow", version); > > ret = -ENOTSUP; > > goto fail; > > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > > struct stat st; > > int64_t image_sectors = 0; > > const char *image_filename = NULL; > > - Error *local_err = NULL; > > int ret; > > BlockDriverState *cow_bs; > > > > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > > options++; > > } > > > > - ret = bdrv_create_file(filename, options, &local_err); > > + ret = bdrv_create_file(filename, options, errp); > > if (ret < 0) { > > - qerror_report_err(local_err); > > - error_free(local_err); > > return ret; > > } > > > > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, > > - &local_err); > > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); > > if (ret < 0) { > > - qerror_report_err(local_err); > > - error_free(local_err); > > return ret; > > } > > This is technically correct, but I think general policy is that using > the local_err pattern is preferred anyway. > If I recall correct, I think there are several places that pass errp along. How about this for a rule of thumb policy: use the local_err method if the function does not indicate error outside of the passed Error pointer.
Il 14/02/2014 17:45, Kevin Wolf ha scritto: >> > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, >> > - &local_err); >> > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); >> > if (ret < 0) { >> > - qerror_report_err(local_err); >> > - error_free(local_err); >> > return ret; >> > } > This is technically correct, but I think general policy is that using > the local_err pattern is preferred anyway. I think only for void-returning functions. I checked callers of monitor_get_fd and they use this pattern, even if they pass &local_err instead of errp. Paolo
Am 14.02.2014 um 18:02 hat Paolo Bonzini geschrieben: > Il 14/02/2014 17:45, Kevin Wolf ha scritto: > >>> - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, > >>> - &local_err); > >>> + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); > >>> if (ret < 0) { > >>> - qerror_report_err(local_err); > >>> - error_free(local_err); > >>> return ret; > >>> } > >This is technically correct, but I think general policy is that using > >the local_err pattern is preferred anyway. > > I think only for void-returning functions. I checked callers of > monitor_get_fd and they use this pattern, even if they pass > &local_err instead of errp. Eventually this function will return void; having both a -errno return and the errp argument is just an intermediate step (as probably in all other cases). So I still think this is going in the wrong direction and will make the conversion harder than necessary. Now that this patch series is already here, I won't insist on respinning it, but please be aware that you're just creating additional work for other people and keep existing local_errs in any future patches. Kevin
Il 14/02/2014 19:19, Kevin Wolf ha scritto: > Eventually this function will return void; having both a -errno return > and the errp argument is just an intermediate step (as probably in all > other cases). So I still think this is going in the wrong direction and > will make the conversion harder than necessary. Now that this patch > series is already here, I won't insist on respinning it, but please > be aware that you're just creating additional work for other people and > keep existing local_errs in any future patches. Ok, this makes sense, but it is an exception to the general policy. :) I'll respin v3 and test NBD too. Paolo
Jeff Cody <jcody@redhat.com> writes: > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote: >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben: >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> > --- >> > block/cow.c | 12 +++--------- >> > 1 file changed, 3 insertions(+), 9 deletions(-) >> > >> > diff --git a/block/cow.c b/block/cow.c >> > index 7fc0b12..43a2150 100644 >> > --- a/block/cow.c >> > +++ b/block/cow.c >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, >> > char version[64]; >> > snprintf(version, sizeof(version), >> > "COW version %d", cow_header.version); >> > - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, >> > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, >> > bs->device_name, "cow", version); >> > ret = -ENOTSUP; >> > goto fail; >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, >> > struct stat st; >> > int64_t image_sectors = 0; >> > const char *image_filename = NULL; >> > - Error *local_err = NULL; >> > int ret; >> > BlockDriverState *cow_bs; >> > >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, >> > options++; >> > } >> > >> > - ret = bdrv_create_file(filename, options, &local_err); >> > + ret = bdrv_create_file(filename, options, errp); >> > if (ret < 0) { >> > - qerror_report_err(local_err); >> > - error_free(local_err); >> > return ret; >> > } >> > >> > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, >> > - &local_err); >> > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); >> > if (ret < 0) { >> > - qerror_report_err(local_err); >> > - error_free(local_err); >> > return ret; >> > } >> >> This is technically correct, but I think general policy is that using >> the local_err pattern is preferred anyway. >> > > If I recall correct, I think there are several places that pass errp > along. How about this for a rule of thumb policy: use the local_err > method if the function does not indicate error outside of the passed > Error pointer. Use &local_err when you need to examine the error object. Passing errp directly is no good then, because it may be null. When you're forwarding errors without examining them, then passing errp directly is just fine.
On Sat, 02/15 11:01, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> writes: > > > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote: > >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben: > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> > --- > >> > block/cow.c | 12 +++--------- > >> > 1 file changed, 3 insertions(+), 9 deletions(-) > >> > > >> > diff --git a/block/cow.c b/block/cow.c > >> > index 7fc0b12..43a2150 100644 > >> > --- a/block/cow.c > >> > +++ b/block/cow.c > >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, > >> > char version[64]; > >> > snprintf(version, sizeof(version), > >> > "COW version %d", cow_header.version); > >> > - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > >> > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > >> > bs->device_name, "cow", version); > >> > ret = -ENOTSUP; > >> > goto fail; > >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > >> > struct stat st; > >> > int64_t image_sectors = 0; > >> > const char *image_filename = NULL; > >> > - Error *local_err = NULL; > >> > int ret; > >> > BlockDriverState *cow_bs; > >> > > >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > >> > options++; > >> > } > >> > > >> > - ret = bdrv_create_file(filename, options, &local_err); > >> > + ret = bdrv_create_file(filename, options, errp); > >> > if (ret < 0) { > >> > - qerror_report_err(local_err); > >> > - error_free(local_err); > >> > return ret; > >> > } > >> > > >> > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, > >> > - &local_err); > >> > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); > >> > if (ret < 0) { > >> > - qerror_report_err(local_err); > >> > - error_free(local_err); > >> > return ret; > >> > } > >> > >> This is technically correct, but I think general policy is that using > >> the local_err pattern is preferred anyway. > >> > > > > If I recall correct, I think there are several places that pass errp > > along. How about this for a rule of thumb policy: use the local_err > > method if the function does not indicate error outside of the passed > > Error pointer. > > Use &local_err when you need to examine the error object. Passing errp > directly is no good then, because it may be null. > > When you're forwarding errors without examining them, then passing errp > directly is just fine. > Does this mean that error_is_set() is always used by programmer to check a non-NULL error pointer? Is there any case to call error_is_set(errp) without knowing if errp is NULL or not? If no, should we enforce the rule and add assert(errp) in error_is_set()? Thanks, Fam
Il 17/02/2014 14:15, Fam Zheng ha scritto: > Does this mean that error_is_set() is always used by programmer to check a > non-NULL error pointer? Is there any case to call error_is_set(errp) without > knowing if errp is NULL or not? If no, should we enforce the rule and add > assert(errp) in error_is_set()? I think we shouldn't need error_is_set() at all... Paolo
On Mon, Feb 17, 2014 at 02:20:10PM +0100, Paolo Bonzini wrote: > Il 17/02/2014 14:15, Fam Zheng ha scritto: > >Does this mean that error_is_set() is always used by programmer to check a > >non-NULL error pointer? Is there any case to call error_is_set(errp) without > >knowing if errp is NULL or not? If no, should we enforce the rule and add > >assert(errp) in error_is_set()? > > I think we shouldn't need error_is_set() at all... > By this do you mean the caller should dereference errp explicitly to check to see if an error is set, or that there should not be void functions that only indicate error via errp?
Am 17.02.2014 um 14:15 hat Fam Zheng geschrieben: > On Sat, 02/15 11:01, Markus Armbruster wrote: > > Jeff Cody <jcody@redhat.com> writes: > > > > > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote: > > >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben: > > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > >> > --- > > >> > block/cow.c | 12 +++--------- > > >> > 1 file changed, 3 insertions(+), 9 deletions(-) > > >> > > > >> > diff --git a/block/cow.c b/block/cow.c > > >> > index 7fc0b12..43a2150 100644 > > >> > --- a/block/cow.c > > >> > +++ b/block/cow.c > > >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, > > >> > char version[64]; > > >> > snprintf(version, sizeof(version), > > >> > "COW version %d", cow_header.version); > > >> > - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > > >> > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > > >> > bs->device_name, "cow", version); > > >> > ret = -ENOTSUP; > > >> > goto fail; > > >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > > >> > struct stat st; > > >> > int64_t image_sectors = 0; > > >> > const char *image_filename = NULL; > > >> > - Error *local_err = NULL; > > >> > int ret; > > >> > BlockDriverState *cow_bs; > > >> > > > >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, > > >> > options++; > > >> > } > > >> > > > >> > - ret = bdrv_create_file(filename, options, &local_err); > > >> > + ret = bdrv_create_file(filename, options, errp); > > >> > if (ret < 0) { > > >> > - qerror_report_err(local_err); > > >> > - error_free(local_err); > > >> > return ret; > > >> > } > > >> > > > >> > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, > > >> > - &local_err); > > >> > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); > > >> > if (ret < 0) { > > >> > - qerror_report_err(local_err); > > >> > - error_free(local_err); > > >> > return ret; > > >> > } > > >> > > >> This is technically correct, but I think general policy is that using > > >> the local_err pattern is preferred anyway. > > >> > > > > > > If I recall correct, I think there are several places that pass errp > > > along. How about this for a rule of thumb policy: use the local_err > > > method if the function does not indicate error outside of the passed > > > Error pointer. > > > > Use &local_err when you need to examine the error object. Passing errp > > directly is no good then, because it may be null. > > > > When you're forwarding errors without examining them, then passing errp > > directly is just fine. > > > > Does this mean that error_is_set() is always used by programmer to check a > non-NULL error pointer? Is there any case to call error_is_set(errp) without > knowing if errp is NULL or not? If no, should we enforce the rule and add > assert(errp) in error_is_set()? Sounds like a good idea to me, it would catch bugs where you forget to use a local_err. Of course, it requires that error_is_set() is used instead of just using errp as a boolean, but such an assertion could actually be a reason to make this the policy. Kevin
Fam Zheng <famz@redhat.com> writes: > On Sat, 02/15 11:01, Markus Armbruster wrote: >> Jeff Cody <jcody@redhat.com> writes: >> >> > On Fri, Feb 14, 2014 at 05:45:40PM +0100, Kevin Wolf wrote: >> >> Am 11.02.2014 um 18:03 hat Paolo Bonzini geschrieben: >> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> >> > --- >> >> > block/cow.c | 12 +++--------- >> >> > 1 file changed, 3 insertions(+), 9 deletions(-) >> >> > >> >> > diff --git a/block/cow.c b/block/cow.c >> >> > index 7fc0b12..43a2150 100644 >> >> > --- a/block/cow.c >> >> > +++ b/block/cow.c >> >> > @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, >> >> > char version[64]; >> >> > snprintf(version, sizeof(version), >> >> > "COW version %d", cow_header.version); >> >> > - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, >> >> > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, >> >> > bs->device_name, "cow", version); >> >> > ret = -ENOTSUP; >> >> > goto fail; >> >> > @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, >> >> > struct stat st; >> >> > int64_t image_sectors = 0; >> >> > const char *image_filename = NULL; >> >> > - Error *local_err = NULL; >> >> > int ret; >> >> > BlockDriverState *cow_bs; >> >> > >> >> > @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, >> >> > options++; >> >> > } >> >> > >> >> > - ret = bdrv_create_file(filename, options, &local_err); >> >> > + ret = bdrv_create_file(filename, options, errp); >> >> > if (ret < 0) { >> >> > - qerror_report_err(local_err); >> >> > - error_free(local_err); >> >> > return ret; >> >> > } >> >> > >> >> > - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, >> >> > - &local_err); >> >> > + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); >> >> > if (ret < 0) { >> >> > - qerror_report_err(local_err); >> >> > - error_free(local_err); >> >> > return ret; >> >> > } >> >> >> >> This is technically correct, but I think general policy is that using >> >> the local_err pattern is preferred anyway. >> >> >> > >> > If I recall correct, I think there are several places that pass errp >> > along. How about this for a rule of thumb policy: use the local_err >> > method if the function does not indicate error outside of the passed >> > Error pointer. >> >> Use &local_err when you need to examine the error object. Passing errp >> directly is no good then, because it may be null. >> >> When you're forwarding errors without examining them, then passing errp >> directly is just fine. >> > > Does this mean that error_is_set() is always used by programmer to check a > non-NULL error pointer? Is there any case to call error_is_set(errp) without > knowing if errp is NULL or not? If no, should we enforce the rule and add > assert(errp) in error_is_set()? If you know ERRP can't be null, then error_is_set(ERRP) is pointless. Just test *ERRP instead. If ERRP may be null, then error_is_set(ERRP) makes some sense: it saves you spelling out ERRP && *ERRP. Personally, I'd prefer it spelled out, though. Note that testing an Error ** that may be null to answer the question "did this fail" is *wrong*, regardless of whether you use error_is_set() or spell it out.
On Mon, 02/17 14:20, Paolo Bonzini wrote: > Il 17/02/2014 14:15, Fam Zheng ha scritto: > >Does this mean that error_is_set() is always used by programmer to check a > >non-NULL error pointer? Is there any case to call error_is_set(errp) without > >knowing if errp is NULL or not? If no, should we enforce the rule and add > >assert(errp) in error_is_set()? > > I think we shouldn't need error_is_set() at all... > Thinking about both use cases (errp is NULL or not) and I agree to this. Fam
On Mon, 02/17 15:59, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > > On Sat, 02/15 11:01, Markus Armbruster wrote: > > Does this mean that error_is_set() is always used by programmer to check a > > non-NULL error pointer? Is there any case to call error_is_set(errp) without > > knowing if errp is NULL or not? If no, should we enforce the rule and add > > assert(errp) in error_is_set()? > > If you know ERRP can't be null, then error_is_set(ERRP) is pointless. > Just test *ERRP instead. > > If ERRP may be null, then error_is_set(ERRP) makes some sense: it saves > you spelling out ERRP && *ERRP. Personally, I'd prefer it spelled out, > though. So the question is whether the returned boolean has any value to anyboday, if ERRP may be NULL. Fam
diff --git a/block/cow.c b/block/cow.c index 7fc0b12..43a2150 100644 --- a/block/cow.c +++ b/block/cow.c @@ -82,7 +82,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, int flags, char version[64]; snprintf(version, sizeof(version), "COW version %d", cow_header.version); - qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, "cow", version); ret = -ENOTSUP; goto fail; @@ -330,7 +330,6 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, struct stat st; int64_t image_sectors = 0; const char *image_filename = NULL; - Error *local_err = NULL; int ret; BlockDriverState *cow_bs; @@ -344,18 +343,13 @@ static int cow_create(const char *filename, QEMUOptionParameter *options, options++; } - ret = bdrv_create_file(filename, options, &local_err); + ret = bdrv_create_file(filename, options, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); return ret; } - ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, - &local_err); + ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR, errp); if (ret < 0) { - qerror_report_err(local_err); - error_free(local_err); return ret; }
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/cow.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)