[libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
Osier Yang
jyang at redhat.com
Mon Feb 11 11:22:37 UTC 2013
On 2013年02月09日 05:09, John Ferlan wrote:
> On 02/08/2013 08:08 AM, Osier Yang wrote:
>> qemuProcessStart invokes qemuProcessStop when fails, to avoid
>> removing hash entry which belongs to other domain(s), this introduces
>> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
>> sets the bit for the disk only if it's successfully added into the
>> hash table. Thus if the argument is provided for qemuProcessStop, it
>> can't remove the hash entry belongs to other domain(s).
>> ---
>> src/qemu/qemu_conf.c | 5 ++++-
>> src/qemu/qemu_conf.h | 3 ++-
>> src/qemu/qemu_driver.c | 21 +++++++++++----------
>> src/qemu/qemu_migration.c | 14 +++++++-------
>> src/qemu/qemu_process.c | 37 +++++++++++++++++++++++++++++--------
>> src/qemu/qemu_process.h | 3 ++-
>> 6 files changed, 55 insertions(+), 28 deletions(-)
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 3eeea4b..a6162b6 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -773,7 +773,8 @@ cleanup:
>> */
>> int
>> qemuAddSharedDisk(virHashTablePtr sharedDisks,
>> - virDomainDiskDefPtr disk)
>> + virDomainDiskDefPtr disk,
>> + int *added)
>> {
>> size_t *ref = NULL;
>> char *key = NULL;
>> @@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
>> }
>> }
>>
>> + if (added)
>> + *added = 1;
>> VIR_FREE(key);
>> return 0;
>> }
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index 8e84bcf..b8b5275 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
>> virConnectPtr conn);
>>
>> int qemuAddSharedDisk(virHashTablePtr sharedDisks,
>> - virDomainDiskDefPtr disk)
>> + virDomainDiskDefPtr disk,
>> + int *added)
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>
>> int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 1dc9789..03fe526 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>> goto endjob;
>> }
>>
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
>> @@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>> goto endjob;
>>
>> /* Shut it down */
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
>> virDomainAuditStop(vm, "saved");
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> @@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
>>
>> endjob:
>> if ((ret == 0)&& (flags& VIR_DUMP_CRASH)) {
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
>> virDomainAuditStop(vm, "crashed");
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> @@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
>> }
>>
>> if (virCommandWait(cmd, NULL)< 0) {
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
>> ret = -1;
>> }
>> VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
>> @@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>> }
>>
>> if (ret == 0) {
>> - if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
>> + if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL)< 0)
>> VIR_WARN("Failed to add disk '%s' to shared disk table",
>> disk->src);
>>
>> @@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
>> if (flags& VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
>> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
>> virDomainAuditStop(vm, "from-snapshot");
>> /* We already filtered the _HALT flag for persistent domains
>> * only, so this end job never drops the last reference. */
>> @@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>>
>> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
>> virDomainAuditStop(vm, "from-snapshot");
>> /* We already filtered the _HALT flag for persistent domains
>> * only, so this end job never drops the last reference. */
>> @@ -12151,8 +12151,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>> goto endjob;
>> }
>> virResetError(err);
>> - qemuProcessStop(driver, vm,
>> - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
>> + 0, NULL);
>> virDomainAuditStop(vm, "from-snapshot");
>> detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
>> event = virDomainEventNewFromObj(vm,
>> @@ -12265,7 +12265,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>
>> if (virDomainObjIsActive(vm)) {
>> /* Transitions 4, 7 */
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
>> + 0, NULL);
>> virDomainAuditStop(vm, "from-snapshot");
>> detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
>> event = virDomainEventNewFromObj(vm,
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index a75fb4e..365afe3 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -1692,7 +1692,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>> virReportSystemError(errno, "%s",
>> _("cannot pass pipe for tunnelled migration"));
>> virDomainAuditStart(vm, "migrated", false);
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
>> goto endjob;
>> }
>> dataFD[1] = -1; /* 'st' owns the FD now& will close it */
>> @@ -3057,7 +3057,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
>> */
>> if (!v3proto) {
>> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(vm, "migrated");
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> @@ -3359,7 +3359,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>> if (!(flags& VIR_MIGRATE_OFFLINE)) {
>> if (qemuMigrationVPAssociatePortProfiles(vm->def)< 0) {
>> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(vm, "failed");
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> @@ -3398,7 +3398,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>> if (v3proto) {
>> if (!(flags& VIR_MIGRATE_OFFLINE)) {
>> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(vm, "failed");
>> }
>> if (newVM)
>> @@ -3446,7 +3446,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>> */
>> if (v3proto) {
>> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(vm, "failed");
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> @@ -3483,7 +3483,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>> qemuProcessAutoDestroyRemove(driver, vm);
>> } else if (!(flags& VIR_MIGRATE_OFFLINE)) {
>> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(vm, "failed");
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> @@ -3554,7 +3554,7 @@ int qemuMigrationConfirm(virQEMUDriverPtr driver,
>> */
>> if (retcode == 0) {
>> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(vm, "migrated");
>>
>> event = virDomainEventNewFromObj(vm,
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 1e0876c..f820c57 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -333,7 +333,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>> event = virDomainEventNewFromObj(vm,
>> VIR_DOMAIN_EVENT_STOPPED,
>> eventReason);
>> - qemuProcessStop(driver, vm, stopReason, 0);
>> + qemuProcessStop(driver, vm, stopReason, 0, NULL);
>> virDomainAuditStop(vm, auditReason);
>>
>> if (!vm->persistent) {
>> @@ -3340,7 +3340,7 @@ error:
>> * really is and FAILED means "failed to start" */
>> state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
>> }
>> - qemuProcessStop(driver, obj, state, 0);
>> + qemuProcessStop(driver, obj, state, 0, NULL);
>> if (!obj->persistent)
>> qemuDomainRemoveInactive(driver, obj);
>> else
>> @@ -3417,7 +3417,8 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
>> } else if (virObjectUnref(obj)) {
>> /* We can't spawn a thread and thus connect to monitor.
>> * Kill qemu */
>> - qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0);
>> + qemuProcessStop(src->driver, obj,
>> + VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
>> if (!obj->persistent)
>> qemuDomainRemoveInactive(src->driver, obj);
>> else
>> @@ -3507,6 +3508,7 @@ int qemuProcessStart(virConnectPtr conn,
>> virBitmapPtr nodemask = NULL;
>> unsigned int stop_flags;
>> virQEMUDriverConfigPtr cfg;
>> + virBitmapPtr added_disks = NULL;
>>
>> /* Okay, these are just internal flags,
>> * but doesn't hurt to check */
>> @@ -3820,9 +3822,15 @@ int qemuProcessStart(virConnectPtr conn,
>> if (cfg->clearEmulatorCapabilities)
>> virCommandClearCaps(cmd);
>>
>> + if (!(added_disks = virBitmapNew(vm->def->ndisks))) {
>> + virReportOOMError();
>> + goto cleanup;
>> + }
>> +
>> /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
>> for (i = 0; i< vm->def->ndisks; i++) {
>> virDomainDiskDefPtr disk = vm->def->disks[i];
>> + int added;
>>
>> if (vm->def->disks[i]->rawio == 1)
>> #ifdef CAP_SYS_RAWIO
>> @@ -3832,8 +3840,10 @@ int qemuProcessStart(virConnectPtr conn,
>> _("Raw I/O is not supported on this platform"));
>> #endif
>>
>> - if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
>> + if (qemuAddSharedDisk(driver->sharedDisks, disk,&added)< 0)
>> goto cleanup;
>> + if (added)
>> + ignore_value(virBitmapSetBit(added_disks, i));
>>
>> if (qemuSetUnprivSGIO(disk)< 0)
>> goto cleanup;
>> @@ -4049,6 +4059,7 @@ int qemuProcessStart(virConnectPtr conn,
>> }
>>
>> virCommandFree(cmd);
>> + virBitmapFree(added_disks);
>> VIR_FORCE_CLOSE(logfile);
>> virObjectUnref(cfg);
>>
>> @@ -4062,7 +4073,8 @@ cleanup:
>> virBitmapFree(nodemask);
>> virCommandFree(cmd);
>> VIR_FORCE_CLOSE(logfile);
>> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
>> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
>> + stop_flags, added_disks);
>
> Do your virBitmapFree(added_disks) here not in called function...
Agreed.
>
>> virObjectUnref(cfg);
>>
>> return -1;
>> @@ -4113,7 +4125,8 @@ qemuProcessKill(virQEMUDriverPtr driver,
>> void qemuProcessStop(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> virDomainShutoffReason reason,
>> - unsigned int flags)
>> + unsigned int flags,
>> + virBitmapPtr added_disks)
>> {
>> int ret;
>> int retries = 0;
>> @@ -4239,9 +4252,17 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>>
>> for (i = 0; i< vm->def->ndisks; i++) {
>> virDomainDiskDefPtr disk = vm->def->disks[i];
>> + bool is_added;
>
> Initialize to false, since result can be unchanged if :
>
> int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result)
> {
> if (bitmap->max_bit<= b)
> return -1;
Okay.
>
>>
>> - ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
>> + if (added_disks) {
>> + ignore_value(virBitmapGetBit(added_disks, i,&is_added));
>> + if (is_added)
>> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
>> + } else {
>> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
>> + }
>> }
>> + virBitmapFree(added_disks);
>
> Let the caller do this. Think of it this way, if you make this call -
> the local added_disks ends up NULL, but that is not reflected back in
> the caller since this was passed by value and not reference. For some
> this is a coding style - caller did the alloc and it should do the free.
Agreed, freeing after call is more clear.
>
>>
>> /* Clear out dynamically assigned labels */
>> for (i = 0; i< vm->def->nseclabels; i++) {
>> @@ -4594,7 +4615,7 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver,
>>
>> VIR_DEBUG("Killing domain");
>> qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED,
>> - VIR_QEMU_PROCESS_STOP_MIGRATED);
>> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>> virDomainAuditStop(dom, "destroyed");
>> event = virDomainEventNewFromObj(dom,
>> VIR_DOMAIN_EVENT_STOPPED,
>> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
>> index 7c620d4..c4f8a6a 100644
>> --- a/src/qemu/qemu_process.h
>> +++ b/src/qemu/qemu_process.h
>> @@ -68,7 +68,8 @@ typedef enum {
>> void qemuProcessStop(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> virDomainShutoffReason reason,
>> - unsigned int flags);
>> + unsigned int flags,
>> + virBitmapPtr added_disks);
>>
>> int qemuProcessAttach(virConnectPtr conn,
>> virQEMUDriverPtr driver,
>>
>
> This is quite a bit of churn for the one place you needed the bitmap.
> Would there be need/cause to add a field to the driver struct with the
> shared disk bitmap rather than a parameter to qemuProcessStop(). Perhaps
> there'd be other uses for it too (just thinking out loud late on snowy
> Friday afternoon).
Don't agree. As it's only used by qemuProcessStart. And it's per-VM
basis, not worth to maintain it in the driver life cycle.
Osier
More information about the libvir-list
mailing list