[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