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...
> 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;
>
> - 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.
>
> /* 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).
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list