[Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Jeff Cody]

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)) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Markus Armbruster]

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>

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Fam Zheng]

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>

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Stefan Hajnoczi]

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.

Re: [Qemu-devel] [PATCH] block: mirror - insure that errp is not NULL

[Stefan Hajnoczi]

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.