[libvirt] [PATCH] qemu: fix use a nonexist address in qemuDomainAttachDeviceConfig
We free them before, then use it. This make we always do virDomainDefAddImplicitControllers when attach a disk. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df3ba6d..a9afa5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7223,10 +7223,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (virDomainDiskInsert(vmdef, disk)) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ - dev->data.disk = NULL; if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; + dev->data.disk = NULL; if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; break; -- 1.8.3.1
On 12/15/14 10:10, Luyao Huang wrote:
We free them before, then use it. This make we always do
We clear the pointer, not free.
virDomainDefAddImplicitControllers when attach a disk.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df3ba6d..a9afa5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7223,10 +7223,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (virDomainDiskInsert(vmdef, disk)) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ - dev->data.disk = NULL;
The comment even explains why the line is there. From that point on, the pointer is owned by 'vmdef' and thus the original needs to be cleared so that it isn't freed (and doulbe freed) later on.
if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
Additionally, 'disk' contains still a valid copy of the pointer so the code should run just fine.
if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; + dev->data.disk = NULL;
So moving the line here doesn't make sense.
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; break;
NACK, Peter
On 12/15/14 10:10, Luyao Huang wrote:
We free them before, then use it. This make we always do We clear the pointer, not free.
virDomainDefAddImplicitControllers when attach a disk.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df3ba6d..a9afa5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7223,10 +7223,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (virDomainDiskInsert(vmdef, disk)) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ - dev->data.disk = NULL; The comment even explains why the line is there. From that point on, the pointer is owned by 'vmdef' and thus the original needs to be cleared so that it isn't freed (and doulbe freed) later on. Oh, i see. Thanks for your review and i found seems i don't check if gdb is real go to virDomainDefAddImplicitControllers, just check the
On 12/16/2014 08:00 PM, Peter Krempa wrote: pointer's value is optimized out. (gdb) 7204 dev->data.disk = NULL; (gdb) 7205 if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) (gdb) p disk $3 = <optimized out> Thanks a lot for your help :)
if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
Additionally, 'disk' contains still a valid copy of the pointer so the code should run just fine.
if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; + dev->data.disk = NULL;
So moving the line here doesn't make sense.
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) return -1; break;
NACK, Peter
Luyao
On 12/16/14 13:32, Luyao Huang wrote:
On 12/15/14 10:10, Luyao Huang wrote:
We free them before, then use it. This make we always do We clear the pointer, not free.
virDomainDefAddImplicitControllers when attach a disk.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df3ba6d..a9afa5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7223,10 +7223,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (virDomainDiskInsert(vmdef, disk)) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ - dev->data.disk = NULL; The comment even explains why the line is there. From that point on, the pointer is owned by 'vmdef' and thus the original needs to be cleared so that it isn't freed (and doulbe freed) later on. Oh, i see. Thanks for your review and i found seems i don't check if gdb is real go to virDomainDefAddImplicitControllers, just check the
On 12/16/2014 08:00 PM, Peter Krempa wrote: pointer's value is optimized out.
(gdb) 7204 dev->data.disk = NULL; (gdb) 7205 if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) (gdb) p disk $3 = <optimized out>
"optimized out" means that GDB can't access the variable as it is stored in a register or for other reason caused by the code optimizer. It does not mean that the variable is cleared or whatever, just that GDB can't print it. Peter
On 12/16/2014 08:33 PM, Peter Krempa wrote:
On 12/16/14 13:32, Luyao Huang wrote:
On 12/15/14 10:10, Luyao Huang wrote:
We free them before, then use it. This make we always do We clear the pointer, not free.
virDomainDefAddImplicitControllers when attach a disk.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df3ba6d..a9afa5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7223,10 +7223,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, if (virDomainDiskInsert(vmdef, disk)) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ - dev->data.disk = NULL; The comment even explains why the line is there. From that point on, the pointer is owned by 'vmdef' and thus the original needs to be cleared so that it isn't freed (and doulbe freed) later on. Oh, i see. Thanks for your review and i found seems i don't check if gdb is real go to virDomainDefAddImplicitControllers, just check the
On 12/16/2014 08:00 PM, Peter Krempa wrote: pointer's value is optimized out.
(gdb) 7204 dev->data.disk = NULL; (gdb) 7205 if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) (gdb) p disk $3 = <optimized out> "optimized out" means that GDB can't access the variable as it is stored in a register or for other reason caused by the code optimizer.
It does not mean that the variable is cleared or whatever, just that GDB can't print it. Got it, thanks in advance for your help. I should be more carefully when i wrote the patch.
Peter
Luyao
participants (2)
-
Luyao Huang -
Peter Krempa