Re: [PATCH] qemu: code protection for qemuBlockJobEventProcessLegacy

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux