| Message ID | 6994360a48564c05021509bf10dbe472ba7f61d6.1392234225.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/12/2014 12:46 PM, Jeff Cody wrote: > When starting a block job, commit_active_start() relies on whether *errp > is set by mirror_start_job. This allows it to determine if the mirror > job start failed, so that it can clean up any changes to open flags from > the bdrv_reopen(). If errp is NULL, then it will not be able to > determine if mirror_start_job failed or not. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/mirror.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 2a43334..41bb83c 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > int64_t length, base_length; > int orig_base_flags; > > + assert(errp != NULL); > + assert(errp); is shorter, but I don't know if we have a preference for implicit conversion of pointers to bool context. Reviewed-by: Eric Blake <eblake@redhat.com>
Jeff Cody <jcody@redhat.com> writes: > When starting a block job, commit_active_start() relies on whether *errp > is set by mirror_start_job. This allows it to determine if the mirror > job start failed, so that it can clean up any changes to open flags from > the bdrv_reopen(). If errp is NULL, then it will not be able to > determine if mirror_start_job failed or not. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/mirror.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 2a43334..41bb83c 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > int64_t length, base_length; > int orig_base_flags; > > + assert(errp != NULL); > + > orig_base_flags = bdrv_get_flags(base); > > if (bdrv_reopen(base, bs->open_flags, errp)) { Needed, because: mirror_start_job(bs, base, speed, 0, 0, on_error, on_error, cb, opaque, errp, &commit_active_job_driver, false, base); if (error_is_set(errp)) { When your callers may legitimately ignore errors, you have to do something like local_err = NULL; mirror_start_job(bs, base, speed, 0, 0, on_error, on_error, cb, opaque, &local_err, &commit_active_job_driver, false, base); error_propagate(errp, local_err); if (local_err) { But here, the assertion should do fine. Reviewed-by: Markus Armbruster <armbru@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 02/12/2014 12:46 PM, Jeff Cody wrote: >> When starting a block job, commit_active_start() relies on whether *errp >> is set by mirror_start_job. This allows it to determine if the mirror >> job start failed, so that it can clean up any changes to open flags from >> the bdrv_reopen(). If errp is NULL, then it will not be able to >> determine if mirror_start_job failed or not. >> >> Reported-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Jeff Cody <jcody@redhat.com> >> --- >> block/mirror.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index 2a43334..41bb83c 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, >> int64_t length, base_length; >> int orig_base_flags; >> >> + assert(errp != NULL); >> + > > assert(errp); is shorter, but I don't know if we have a preference for > implicit conversion of pointers to bool context. I do[*], but as far as I can tell, the project does not. [*] I prefer the implicit conversion.
On Thu, 02/13 07:40, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> writes: > > > When starting a block job, commit_active_start() relies on whether *errp > > is set by mirror_start_job. This allows it to determine if the mirror > > job start failed, so that it can clean up any changes to open flags from > > the bdrv_reopen(). If errp is NULL, then it will not be able to > > determine if mirror_start_job failed or not. > > > > Reported-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/mirror.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/block/mirror.c b/block/mirror.c > > index 2a43334..41bb83c 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > > int64_t length, base_length; > > int orig_base_flags; > > > > + assert(errp != NULL); > > + > > orig_base_flags = bdrv_get_flags(base); > > > > if (bdrv_reopen(base, bs->open_flags, errp)) { > > Needed, because: > > mirror_start_job(bs, base, speed, 0, 0, > on_error, on_error, cb, opaque, errp, > &commit_active_job_driver, false, base); > if (error_is_set(errp)) { > > When your callers may legitimately ignore errors, you have to do > something like > > local_err = NULL; > mirror_start_job(bs, base, speed, 0, 0, > on_error, on_error, cb, opaque, &local_err, > &commit_active_job_driver, false, base); > error_propagate(errp, local_err); > if (local_err) { > > But here, the assertion should do fine. Thanks for the explanation! Reviewed-by: Fam Zheng <famz@redhat.com>
Am 12.02.2014 um 20:46 hat Jeff Cody geschrieben: > When starting a block job, commit_active_start() relies on whether *errp > is set by mirror_start_job. This allows it to determine if the mirror > job start failed, so that it can clean up any changes to open flags from > the bdrv_reopen(). If errp is NULL, then it will not be able to > determine if mirror_start_job failed or not. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/mirror.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 2a43334..41bb83c 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > int64_t length, base_length; > int orig_base_flags; > > + assert(errp != NULL); > + > orig_base_flags = bdrv_get_flags(base); > > if (bdrv_reopen(base, bs->open_flags, errp)) { This is surprising behaviour. Without looking at the function implementation, I expect that errp == NULL works and means the same as everywhere else: The caller doesn't care about errors. I wouldn't mind if violators were detected at compile time, but this is merely a run-time error (and strictly speaking only an error at all without NDEBUG), so I would prefer to use the normal local_err pattern. Kevin
On Thu, Feb 13, 2014 at 09:41:45AM +0100, Kevin Wolf wrote: > Am 12.02.2014 um 20:46 hat Jeff Cody geschrieben: > > When starting a block job, commit_active_start() relies on whether *errp > > is set by mirror_start_job. This allows it to determine if the mirror > > job start failed, so that it can clean up any changes to open flags from > > the bdrv_reopen(). If errp is NULL, then it will not be able to > > determine if mirror_start_job failed or not. > > > > Reported-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/mirror.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/block/mirror.c b/block/mirror.c > > index 2a43334..41bb83c 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > > int64_t length, base_length; > > int orig_base_flags; > > > > + assert(errp != NULL); > > + > > orig_base_flags = bdrv_get_flags(base); > > > > if (bdrv_reopen(base, bs->open_flags, errp)) { > > This is surprising behaviour. Without looking at the function > implementation, I expect that errp == NULL works and means the same as > everywhere else: The caller doesn't care about errors. > > I wouldn't mind if violators were detected at compile time, but this is > merely a run-time error (and strictly speaking only an error at all > without NDEBUG), so I would prefer to use the normal local_err pattern. Agreed. Please use local_err.
On Wed, Feb 12, 2014 at 02:46:47PM -0500, Jeff Cody wrote: It doesn't really matter, but: http://public.wsu.edu/~brians/errors/assure.html “ensure” that something happens is to make certain that it does, and to “insure” is to issue an insurance policy. Other authorities, however, consider “ensure” and “insure” interchangeable. To please conservatives, make the distinction.
diff --git a/block/mirror.c b/block/mirror.c index 2a43334..41bb83c 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -634,6 +634,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, int64_t length, base_length; int orig_base_flags; + assert(errp != NULL); + orig_base_flags = bdrv_get_flags(base); if (bdrv_reopen(base, bs->open_flags, errp)) {
When starting a block job, commit_active_start() relies on whether *errp is set by mirror_start_job. This allows it to determine if the mirror job start failed, so that it can clean up any changes to open flags from the bdrv_reopen(). If errp is NULL, then it will not be able to determine if mirror_start_job failed or not. Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/mirror.c | 2 ++ 1 file changed, 2 insertions(+)