On Thu, Jul 01, 2021 at 16:37:29 +0800, Chongyun Wu wrote:
>
>
> On 2021/7/1 15:24, Peter Krempa wrote:
> > On Thu, Jul 01, 2021 at 14:55:56 +0800, huangy81@xxxxxxxxxxxxxxx wrote:
> > > From: Chongyun Wu <wucy11@xxxxxxxxxxxxxxx>
> > >
> > > 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
Well qemu 2.12 is very old at this point, we techincally support it but
if you are using latest libvirt I'd strongly suggest also using a more
modern qemu version.
Also note, that blockcopy to network destinations is supposed to
properly work with qemu 4.2 and newer which uses -blockdev to configure
disks.
> some processes and triggered this crash, and normal use will not have this
You mean you've modified libvirt which lead to a crash?
I'm still curious to what happened to trigger the issue. In many cases a
fix such like this fixes the symptom and not the root cause for the
problem.
> problem. So I just want to do some protection at the code level. Thanks~
>
> >
> > >
> > > Signed-off-by: Chongyun Wu <wucy11@xxxxxxxxxxxxxxx>
> > > ---
> > > 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.
Well in the legacy block job handlers it shouldn't be possible that a
VIR_DOMAIN_BLOCK_JOB_COMPLETED event is delivered when disk is already
NULL. That's why I'm asking what exact steps lead to the crash so that I
can investigate also the possible root cause of the problem.
