[libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
Osier Yang
jyang at redhat.com
Mon Feb 11 10:35:42 UTC 2013
On 2013年02月09日 04:21, John Ferlan wrote:
> On 02/08/2013 08:07 AM, Osier Yang wrote:
>> This moves the checking into the helpers, to avoid the
>> callers missing the checking.
>> ---
>> src/qemu/qemu_conf.c | 20 ++++++++++++++++----
>> src/qemu/qemu_conf.h | 4 ++--
>> src/qemu/qemu_driver.c | 18 +++++++-----------
>> src/qemu/qemu_process.c | 21 ++++++++++++---------
>> 4 files changed, 37 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 17f7d45..69ba710 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
>> */
>> int
>> qemuAddSharedDisk(virHashTablePtr sharedDisks,
>> - const char *disk_path)
>> + virDomainDiskDefPtr disk)
>> {
>> size_t *ref = NULL;
>> char *key = NULL;
>>
>> - if (!(key = qemuGetSharedDiskKey(disk_path)))
>> + /* Currently the only conflicts we have to care about
>> + * for the shared disk is "sgio" setting, which is only
>> + * valid for block disk.
>> + */
>> + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
>> + !disk->shared || !disk->src)
>> + return 0;
>> +
>> + if (!(key = qemuGetSharedDiskKey(disk->src)))
>> return -1;
>>
>> if ((ref = virHashLookup(sharedDisks, key))) {
>> @@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
>> */
>> int
>> qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
>> - const char *disk_path)
>> + virDomainDiskDefPtr disk)
>> {
>> size_t *ref = NULL;
>> char *key = NULL;
>>
>> - if (!(key = qemuGetSharedDiskKey(disk_path)))
>> + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
>> + !disk->shared || !disk->src)
>> + return 0;
[2]
>> +
>> + if (!(key = qemuGetSharedDiskKey(disk->src)))
>> return -1;
>>
>> if (!(ref = virHashLookup(sharedDisks, key))) {
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index 60c4109..8e84bcf 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
>> virConnectPtr conn);
>>
>> int qemuAddSharedDisk(virHashTablePtr sharedDisks,
>> - const char *disk_path)
>> + virDomainDiskDefPtr disk)
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>
>> int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
>> - const char *disk_path)
>> + virDomainDiskDefPtr disk)
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>> char * qemuGetSharedDiskKey(const char *disk_path)
>> ATTRIBUTE_NONNULL(1);
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 979a027..043efd3 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>> }
>>
>> if (ret == 0) {
>> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared) {
>> - if (qemuAddSharedDisk(driver->sharedDisks, disk->src)< 0)
>> - VIR_WARN("Failed to add disk '%s' to shared disk table",
>> - disk->src);
>> - }
>> + if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
>> + VIR_WARN("Failed to add disk '%s' to shared disk table",
>> + disk->src);
>>
>> if (qemuSetUnprivSGIO(disk)< 0)
>> VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);
>
> Does there need to be a NULLSTR(disk->src)? Doesn't seem so, but just
> checking. I only note this because the qemuAddSharedDisk() has a
> !disk->src check prior to returning zero.
qemuSetUnprivSGIO returns 0 as long as disk->src is NULL too. See [1].
>
>> @@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
>> break;
>> }
>>
>> - if (ret == 0&&
>> - disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&&
>> - disk->shared) {
>> - if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)< 0)
>> - VIR_WARN("Failed to remove disk '%s' from shared disk table",
>> - disk->src);
>> + if (ret == 0) {
>> + if (qemuRemoveSharedDisk(driver->sharedDisks, disk)< 0)
>> + VIR_WARN("Failed to remove disk '%s' from shared disk table",
>> + disk->src);
>
> Similar comment here w/r/t NULLSTR and checks in Remove code
Likewise. See [2].
>
>> }
>>
>> return ret;
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 30f923a..bc171a4 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
>> {
>> int val = -1;
>>
>> + /* "sgio" is only valid for block disk; cdrom
>> + * and floopy disk can have empty source.
>> + */
>> + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
>> + !disk->src)
>> + return 0;
[1]
>> +
>> if (disk->sgio)
>> val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
>>
>> @@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
>> _("Raw I/O is not supported on this platform"));
>> #endif
>>
>> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared) {
>> - if (qemuAddSharedDisk(driver->sharedDisks, disk->src)< 0)
>> - goto cleanup;
>> + if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
>> + goto cleanup;
>>
>> - if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0)
>> - goto cleanup;
>> - }
>> + if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0)
>> + goto cleanup;
>>
>> if (qemuSetUnprivSGIO(disk)< 0)
>> goto cleanup;
>> @@ -4288,9 +4293,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>> for (i = 0; i< vm->def->ndisks; i++) {
>> virDomainDiskDefPtr disk = vm->def->disks[i];
>>
>> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared) {
>> - ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
>> - }
>> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
>> }
>>
>> /* Clear out dynamically assigned labels */
>>
>
> ACK - everything seems straightforward to me.
>
> John
>
> --
> 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