| Message ID | 1466587008-3933-2-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/06/2016 11:16, Changlong Xie wrote: > Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> > --- > block/commit.c | 1 + > block/mirror.c | 2 ++ > block/stream.c | 1 + > 3 files changed, 4 insertions(+) Why is this useful? Paolo > diff --git a/block/commit.c b/block/commit.c > index 444333b..13b55c1 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -223,6 +223,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, > BlockDriverState *overlay_bs; > Error *local_err = NULL; > > + assert(cb); > assert(top != bs); > if (top == base) { > error_setg(errp, "Invalid files for merge: top and base are the same"); > diff --git a/block/mirror.c b/block/mirror.c > index a04ed9c..fa2bdab 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -912,6 +912,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > bool is_none_mode; > BlockDriverState *base; > > + assert(cb); > if (mode == MIRROR_SYNC_MODE_INCREMENTAL) { > error_setg(errp, "Sync mode 'incremental' not supported"); > return; > @@ -935,6 +936,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > int ret; > Error *local_err = NULL; > > + assert(cb); > orig_base_flags = bdrv_get_flags(base); > > if (bdrv_reopen(base, bs->open_flags, errp)) { > diff --git a/block/stream.c b/block/stream.c > index c0efbda..fc34c63 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -226,6 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, > { > StreamBlockJob *s; > > + assert(cb); > s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp); > if (!s) { > return; >
On 06/22/2016 05:50 PM, Paolo Bonzini wrote: > > > On 22/06/2016 11:16, Changlong Xie wrote: >> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >> --- >> block/commit.c | 1 + >> block/mirror.c | 2 ++ >> block/stream.c | 1 + >> 3 files changed, 4 insertions(+) > > Why is this useful? > commit/mirror/stream/backup use block_job_create(..., cb,..) to create relevant blockjob. When they finished, these jobs will invoke block_job_completed, then invoke job->cb() unconditionally. So i think we need this to avoid segment fault. Actually backup has implemented this. Thanks -Xie > Paolo > >> diff --git a/block/commit.c b/block/commit.c >> index 444333b..13b55c1 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -223,6 +223,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, >> BlockDriverState *overlay_bs; >> Error *local_err = NULL; >> >> + assert(cb); >> assert(top != bs); >> if (top == base) { >> error_setg(errp, "Invalid files for merge: top and base are the same"); >> diff --git a/block/mirror.c b/block/mirror.c >> index a04ed9c..fa2bdab 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -912,6 +912,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, >> bool is_none_mode; >> BlockDriverState *base; >> >> + assert(cb); >> if (mode == MIRROR_SYNC_MODE_INCREMENTAL) { >> error_setg(errp, "Sync mode 'incremental' not supported"); >> return; >> @@ -935,6 +936,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, >> int ret; >> Error *local_err = NULL; >> >> + assert(cb); >> orig_base_flags = bdrv_get_flags(base); >> >> if (bdrv_reopen(base, bs->open_flags, errp)) { >> diff --git a/block/stream.c b/block/stream.c >> index c0efbda..fc34c63 100644 >> --- a/block/stream.c >> +++ b/block/stream.c >> @@ -226,6 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, >> { >> StreamBlockJob *s; >> >> + assert(cb); >> s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp); >> if (!s) { >> return; >> > > >
On 22/06/2016 12:12, Changlong Xie wrote: > > commit/mirror/stream/backup use block_job_create(..., cb,..) to create > relevant blockjob. When they finished, these jobs will invoke > block_job_completed, then invoke job->cb() unconditionally. So i think > we need this to avoid segment fault. Actually backup has implemented this. So this suggests that the right place to put the assertion would be block_job_create. But it's even better to add a #define QEMU_NONNULL __attribute__((__nonnull__)) to include/qemu/compiler.h and declare the arguments as non-null. Paolo
On 06/22/2016 04:19 AM, Paolo Bonzini wrote: > > > On 22/06/2016 12:12, Changlong Xie wrote: >> >> commit/mirror/stream/backup use block_job_create(..., cb,..) to create >> relevant blockjob. When they finished, these jobs will invoke >> block_job_completed, then invoke job->cb() unconditionally. So i think >> we need this to avoid segment fault. Actually backup has implemented this. > > So this suggests that the right place to put the assertion would be > block_job_create. But it's even better to add a > > #define QEMU_NONNULL __attribute__((__nonnull__)) > > to include/qemu/compiler.h and declare the arguments as non-null. Or alternatively fix things to only invoke job->cb() if it is non-NULL, so that callers don't have to pass in a no-op callback just to appease a non-NULL attribute.
On 06/23/2016 01:31 AM, Eric Blake wrote: > On 06/22/2016 04:19 AM, Paolo Bonzini wrote: >> >> >> On 22/06/2016 12:12, Changlong Xie wrote: >>> >>> commit/mirror/stream/backup use block_job_create(..., cb,..) to create >>> relevant blockjob. When they finished, these jobs will invoke >>> block_job_completed, then invoke job->cb() unconditionally. So i think >>> we need this to avoid segment fault. Actually backup has implemented this. >> >> So this suggests that the right place to put the assertion would be >> block_job_create. But it's even better to add a >> >> #define QEMU_NONNULL __attribute__((__nonnull__)) >> >> to include/qemu/compiler.h and declare the arguments as non-null. > > Or alternatively fix things to only invoke job->cb() if it is non-NULL, > so that callers don't have to pass in a no-op callback just to appease a > non-NULL attribute. > Is there any reason, that we should invoke job->cb() unconditionally without nonnull check? There is no relevant clue in the historical commit messages. If yes, i prefer paolo's suggestion; otherwise eric's solution is better. Thanks -Xie
Am 23.06.2016 um 03:04 hat Changlong Xie geschrieben: > On 06/23/2016 01:31 AM, Eric Blake wrote: > >On 06/22/2016 04:19 AM, Paolo Bonzini wrote: > >> > >> > >>On 22/06/2016 12:12, Changlong Xie wrote: > >>> > >>>commit/mirror/stream/backup use block_job_create(..., cb,..) to create > >>>relevant blockjob. When they finished, these jobs will invoke > >>>block_job_completed, then invoke job->cb() unconditionally. So i think > >>>we need this to avoid segment fault. Actually backup has implemented this. > >> > >>So this suggests that the right place to put the assertion would be > >>block_job_create. But it's even better to add a > >> > >>#define QEMU_NONNULL __attribute__((__nonnull__)) > >> > >>to include/qemu/compiler.h and declare the arguments as non-null. > > > >Or alternatively fix things to only invoke job->cb() if it is non-NULL, > >so that callers don't have to pass in a no-op callback just to appease a > >non-NULL attribute. > > Is there any reason, that we should invoke job->cb() unconditionally > without nonnull check? There is no relevant clue in the historical > commit messages. If yes, i prefer paolo's suggestion; otherwise > eric's solution is better. I don't think no-op callbacks actually exist for jobs. Kevin
On 06/23/2016 02:21 PM, Kevin Wolf wrote: > Am 23.06.2016 um 03:04 hat Changlong Xie geschrieben: >> On 06/23/2016 01:31 AM, Eric Blake wrote: >>> On 06/22/2016 04:19 AM, Paolo Bonzini wrote: >>>> >>>> >>>> On 22/06/2016 12:12, Changlong Xie wrote: >>>>> >>>>> commit/mirror/stream/backup use block_job_create(..., cb,..) to create >>>>> relevant blockjob. When they finished, these jobs will invoke >>>>> block_job_completed, then invoke job->cb() unconditionally. So i think >>>>> we need this to avoid segment fault. Actually backup has implemented this. >>>> >>>> So this suggests that the right place to put the assertion would be >>>> block_job_create. But it's even better to add a >>>> >>>> #define QEMU_NONNULL __attribute__((__nonnull__)) >>>> >>>> to include/qemu/compiler.h and declare the arguments as non-null. >>> >>> Or alternatively fix things to only invoke job->cb() if it is non-NULL, >>> so that callers don't have to pass in a no-op callback just to appease a >>> non-NULL attribute. >> >> Is there any reason, that we should invoke job->cb() unconditionally >> without nonnull check? There is no relevant clue in the historical >> commit messages. If yes, i prefer paolo's suggestion; otherwise >> eric's solution is better. > > I don't think no-op callbacks actually exist for jobs. > So, i'll put assert(cb) in block_job_create as Paolo suggested. Thanks -Xie > Kevin > > > . >
diff --git a/block/commit.c b/block/commit.c index 444333b..13b55c1 100644 --- a/block/commit.c +++ b/block/commit.c @@ -223,6 +223,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, BlockDriverState *overlay_bs; Error *local_err = NULL; + assert(cb); assert(top != bs); if (top == base) { error_setg(errp, "Invalid files for merge: top and base are the same"); diff --git a/block/mirror.c b/block/mirror.c index a04ed9c..fa2bdab 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -912,6 +912,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, bool is_none_mode; BlockDriverState *base; + assert(cb); if (mode == MIRROR_SYNC_MODE_INCREMENTAL) { error_setg(errp, "Sync mode 'incremental' not supported"); return; @@ -935,6 +936,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, int ret; Error *local_err = NULL; + assert(cb); orig_base_flags = bdrv_get_flags(base); if (bdrv_reopen(base, bs->open_flags, errp)) { diff --git a/block/stream.c b/block/stream.c index c0efbda..fc34c63 100644 --- a/block/stream.c +++ b/block/stream.c @@ -226,6 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, { StreamBlockJob *s; + assert(cb); s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp); if (!s) { return;
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> --- block/commit.c | 1 + block/mirror.c | 2 ++ block/stream.c | 1 + 3 files changed, 4 insertions(+)