[libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting

[Osier Yang]

On 2013年02月11日 18:48, Daniel P. Berrange wrote:
> On Mon, Feb 11, 2013 at 06:35:42PM +0800, Osier Yang wrote:
>> 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].
>
> If disk->type == NETWORK, then de-referencing disk->src has potential to
> SEGV the entire process, since that field is not valid.

There is checking in this version:

     /* "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;

So for a network disk, it has no chance to de-reference disk->src
if the return value < 0.

>>
>>>
>>>> @@ -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].
>
> Again you must *not* de-reference disk->src without first validating
> the disk->type value.
>

Likewise.

>
> Daniel