[libvirt] [PATCH 3/6] qemu: Record names of domain which uses the shared disk in hash table
Osier Yang
jyang at redhat.com
Wed Feb 20 05:30:08 UTC 2013
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list