Re: [PATCH] qemu: code protection for qemuBlockJobEventProcessLegacy

On 2021/7/1 15:24, Peter Krempa wrote: > On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81(a)chinatelecom.cn wrote: > > From: Chongyun Wu <wucy11(a)chinatelecom.cn&gt; > > > > pointer disk might be null in some special cases or new > > usage scenarios, therefore code protection is needed to > > prevent segment faults. > > Please elaborate on when that happens. Generally the legacy block job > handler should no longer be in action with new versions, elaborate > please also where you are seeing this. Thanks for your comments. I found it when I develop a new feature which make quemu2.12 support the rbd block hot migration with blockcopy command, I changed

some processes and triggered this crash, and normal use will not have this

problem. So I just want to do some protection at the code level. Thanks~ > > > > > Signed-off-by: Chongyun Wu <wucy11(a)chinatelecom.cn&gt; > > --- > > src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++++++++------------------- > > 1 file changed, 24 insertions(+), 19 deletions(-) > > > > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c > > index faf9a9f..00506b9 100644 > > --- a/src/qemu/qemu_blockjob.c > > +++ b/src/qemu/qemu_blockjob.c > > @@ -781,12 +781,13 @@ qemuBlockJobEventProcessLegacy(virQEMUDriver *driver, > > { > > virDomainDiskDef *disk = job->disk; > > - VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d", > > - disk->dst, > > - NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)), > > - job->type, > > - job->state, > > - job->newstate); > > + if (disk) > > + VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, state=%d, newstate=%d", > > + disk->dst, > > + NULLSTR(virDomainDiskMirrorStateTypeToString(disk->mirrorState)), > > + job->type, > > + job->state, > > + job->newstate); > > The legacy block job handler makes no real sense if disk is NULL ... Yes, you are right. The reason I used to do this is that if ob->state is VIR_DOMAIN_BLOCK_JOB_COMPLETED, you can ignore whether the disk is empty. But what you said also makes sens, maybe I can do the judgement at the beginning.