On 2013年02月20日 02:57, John Ferlan wrote:
On 02/19/2013 07:27 AM, Osier Yang wrote:
The hash entry is changed from "ref" to {ref, @domains}. With this, the
caller can simply call qemuRemoveSharedDisk, without afraid of removing
the entry belongs to other domains. qemuProcessStart will obviously
benifit from it on error codepath (which calls qemuProcessStop to do
the cleanup).
---
src/qemu/qemu_conf.c | 163 +++++++++++++++++++++++++++++++++++++++++------
src/qemu/qemu_conf.h | 26 ++++++-
src/qemu/qemu_driver.c | 6 +-
src/qemu/qemu_process.c | 4 +-
4 files changed, 171 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index e54227f..a0477b3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -858,10 +858,9 @@ static int
qemuCheckSharedDisk(virHashTablePtr sharedDisks,
virDomainDiskDefPtr disk)
{
- int val;
- size_t *ref = NULL;
char *sysfs_path = NULL;
char *key = NULL;
+ int val;
int ret = 0;
/* The only conflicts between shared disk we care about now
@@ -881,7 +880,6 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks,
if (!virFileExists(sysfs_path))
goto cleanup;
-
if (!(key = qemuGetSharedDiskKey(disk->src))) {
ret = -1;
goto cleanup;
@@ -890,7 +888,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks,
/* It can't be conflict if no other domain is
* is sharing it.
*/
- if (!(ref = virHashLookup(sharedDisks, key)))
+ if (!(virHashLookup(sharedDisks, key)))
goto cleanup;
if (virGetDeviceUnprivSGIO(disk->src, NULL,&val)< 0) {
@@ -916,14 +914,83 @@ cleanup:
return ret;
}
-/* Increase ref count if the entry already exists, otherwise
- * add a new entry.
+bool
+qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry,
+ const char *name,
+ int *idx)
+{
+ int i;
+
+ for (i = 0; i< entry->ref; i++) {
+ if (STREQ(entry->domains[i], name)) {
+ if (idx)
+ *idx = i;
+ return true;
+ }
+ }
+
+ return false;
+}
+
+void
+qemuSharedDiskEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED)
+{
+ qemuSharedDiskEntryPtr entry = (qemuSharedDiskEntryPtr)payload;
+ int i;
+
+ for (i = 0; i< entry->ref; i++) {
+ VIR_FREE(entry->domains[i]);
+ }
+ VIR_FREE(entry->domains);
Do we need to VIR_FREE(entry)?
Oh, right, I missed it.
There's a VIR_ALLOC(ret) below, followed by VIR_ALLOC_N(ret->domains),
followed by strdup(entry->domains[i]);
+}
+
+static qemuSharedDiskEntryPtr
+qemuSharedDiskEntryCopy(const qemuSharedDiskEntryPtr entry)
+{
+ qemuSharedDiskEntryPtr ret = NULL;
+ int i;
+
+ if (VIR_ALLOC(ret)< 0) {
+ virReportOOMError();
+ return NULL;
+ }
+
+ if (VIR_ALLOC_N(ret->domains, entry->ref)< 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ for (i = 0; i< entry->ref; i++) {
+ if (!(ret->domains[i] = strdup(entry->domains[i]))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ ret->ref++;
+ }
+
+ return ret;
+
+cleanup:
+ qemuSharedDiskEntryFree(ret, NULL);
+ return NULL;
+}
+
+/* qemuAddSharedDisk:
+ * @driver: Pointer to qemu driver struct
+ * @disk: The disk def
+ * @name: The domain name
+ *
+ * Increase ref count and add the domain name into the list which
+ * records all the domains that use the shared disk if the entry
+ * already exists, otherwise add a new entry.
*/
int
qemuAddSharedDisk(virQEMUDriverPtr driver,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ const char *name)
{
- size_t *ref = NULL;
+ qemuSharedDiskEntry *entry = NULL;
+ qemuSharedDiskEntry *new_entry = NULL;
char *key = NULL;
int ret = -1;
@@ -942,11 +1009,40 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
if (!(key = qemuGetSharedDiskKey(disk->src)))
goto cleanup;
- if ((ref = virHashLookup(driver->sharedDisks, key))) {
- if (virHashUpdateEntry(driver->sharedDisks, key, ++ref)< 0)
- goto cleanup;
+ if ((entry = virHashLookup(driver->sharedDisks, key))) {
+ /* Nothing to do if the shared disk is already recorded
+ * in the table.
+ */
+ if (qemuSharedDiskEntryDomainExists(entry, name, NULL)) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!(new_entry = qemuSharedDiskEntryCopy(entry)))
+ goto cleanup;
+
+ if ((VIR_EXPAND_N(new_entry->domains, new_entry->ref, 1)< 0) ||
+ !(new_entry->domains[new_entry->ref - 1] = strdup(name))) {
+ qemuSharedDiskEntryFree(new_entry, NULL);
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ if (virHashUpdateEntry(driver->sharedDisks, key, new_entry)< 0) {
+ qemuSharedDiskEntryFree(new_entry, NULL);
+ goto cleanup;
+ }
If this is successful, then shouldn't the former 'entry' now be
DiskEntryFree'd since you've successfully replaced the former 'entry'
with 'new_entry' in the hash table (eg, payload = new data structure?
virHashUpdateEntry will free the old entry, see:
/**
* virHashUpdateEntry:
* @table: the hash table
* @name: the name of the userdata
* @userdata: a pointer to the userdata
*
* Add the @userdata to the hash @table. This can later be retrieved
* by using @name. Existing entry for this tuple
* will be removed and freed with @f if found.
The comments avoid need to be improved though, (@f is vague here).
But its meaning is table->dataFree, see virHashAddOrUpdateEntry:
if (is_update) {
if (table->dataFree)
table->dataFree(entry->payload, entry->name);
entry->payload = userdata;
return 0;
} else {
- if (virHashAddEntry(driver->sharedDisks, key, (void *)0x1))
+ if ((VIR_ALLOC(entry)< 0) ||
+ (VIR_ALLOC_N(entry->domains, 1)< 0) ||
+ !(entry->domains[0] = strdup(name))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ entry->ref = 1;
+
+ if (virHashAddEntry(driver->sharedDisks, key, entry))
goto cleanup;
}
@@ -957,16 +1053,25 @@ cleanup:
return ret;
}
-/* Decrease the ref count if the entry already exists, otherwise
- * remove the entry.
+/* qemuRemoveSharedDisk:
+ * @driver: Pointer to qemu driver struct
+ * @disk: The disk def
+ * @name: The domain name
+ *
+ * Decrease ref count and remove the domain name from the list which
+ * records all the domains that use the shared disk if ref is not 1,
+ * otherwise remove the entry.
*/
int
qemuRemoveSharedDisk(virQEMUDriverPtr driver,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ const char *name)
{
- size_t *ref = NULL;
+ qemuSharedDiskEntryPtr entry = NULL;
+ qemuSharedDiskEntryPtr new_entry = NULL;
char *key = NULL;
int ret = -1;
+ int idx;
if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
!disk->shared || !disk->src)
@@ -976,12 +1081,32 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
if (!(key = qemuGetSharedDiskKey(disk->src)))
goto cleanup;
- if (!(ref = virHashLookup(driver->sharedDisks, key)))
+ if (!(entry = virHashLookup(driver->sharedDisks, key)))
+ goto cleanup;
+
+ /* Nothing to do if the shared disk is not recored in
+ * the table.
+ */
+ if (!qemuSharedDiskEntryDomainExists(entry, name,&idx)) {
+ ret = 0;
goto cleanup;
+ }
+
+ if (entry->ref != 1) {
+ if (!(new_entry = qemuSharedDiskEntryCopy(entry)))
+ goto cleanup;
+
+ if (idx != new_entry->ref - 1)
+ memmove(&new_entry->domains[idx],
+&new_entry->domains[idx + 1],
+ sizeof(*new_entry->domains) * (new_entry->ref - idx - 1));
- if (ref != (void *)0x1) {
- if (virHashUpdateEntry(driver->sharedDisks, key, --ref)< 0)
+ VIR_SHRINK_N(new_entry->domains, new_entry->ref, 1);
+
+ if (virHashUpdateEntry(driver->sharedDisks, key, new_entry)< 0){
+ qemuSharedDiskEntryFree(new_entry, NULL);
goto cleanup;
+ }
Same comment as above. 'new_entry' is now in the hash - the old 'entry'
can be DiskEntryFree()'d right?
The same.
} else {
if (virHashRemoveEntry(driver->sharedDisks, key)< 0)
goto cleanup;
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 81a001b..d3cd0a1 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -272,16 +272,34 @@ qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virConnectPtr conn);
-int qemuAddSharedDisk(virQEMUDriverPtr driver,
- virDomainDiskDefPtr disk)
+typedef struct _qemuSharedDiskEntry qemuSharedDiskEntry;
+typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr;
+struct _qemuSharedDiskEntry {
+ size_t ref;
+ char **domains; /* array of domain names */
+};
+
+bool qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry,
+ const char *name,
+ int *index)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int qemuAddSharedDisk(virQEMUDriverPtr driver,
+ virDomainDiskDefPtr disk,
+ const char *name)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+
int qemuRemoveSharedDisk(virQEMUDriverPtr driver,
- virDomainDiskDefPtr disk)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ virDomainDiskDefPtr disk,
+ const char *name)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+
char * qemuGetSharedDiskKey(const char *disk_path)
ATTRIBUTE_NONNULL(1);
+void qemuSharedDiskEntryFree(void *payload, const void *name)
+ ATTRIBUTE_NONNULL(1);
+
int qemuDriverAllocateID(virQEMUDriverPtr driver);
#endif /* __QEMUD_CONF_H */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5ebe3fa..27d3daf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -679,7 +679,7 @@ qemuStartup(bool privileged,
if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL)
goto error;
- if (!(qemu_driver->sharedDisks = virHashCreate(30, NULL)))
+ if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDiskEntryFree)))
goto error;
if (privileged) {
@@ -5762,7 +5762,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
}
if (ret == 0) {
- if (qemuAddSharedDisk(driver, disk)< 0)
+ if (qemuAddSharedDisk(driver, disk, vm->def->name)< 0)
VIR_WARN("Failed to add disk '%s' to shared disk table",
disk->src);
@@ -5886,7 +5886,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
}
if (ret == 0) {
- if (qemuRemoveSharedDisk(driver, disk)< 0)
+ if (qemuRemoveSharedDisk(driver, disk, vm->def->name)< 0)
VIR_WARN("Failed to remove disk '%s' from shared disk table",
disk->src);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e955260..69e4209 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3831,7 +3831,7 @@ int qemuProcessStart(virConnectPtr conn,
_("Raw I/O is not supported on this platform"));
#endif
- if (qemuAddSharedDisk(driver, disk)< 0)
+ if (qemuAddSharedDisk(driver, disk, vm->def->name)< 0)
goto cleanup;
if (qemuSetUnprivSGIO(disk)< 0)
@@ -4237,7 +4237,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i< vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
- ignore_value(qemuRemoveSharedDisk(driver, disk));
+ ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name));
}
/* Clear out dynamically assigned labels */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list