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@chinatelecom.cn wrote:
From: Chongyun Wu <wucy11@chinatelecom.cn>
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@chinatelecom.cn> --- 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.