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

[Osier Yang]

On 2013年02月11日 19:14, Daniel P. Berrange wrote:
> On Mon, Feb 11, 2013 at 07:09:29PM +0800, Osier Yang wrote:
>> 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.
>
> Hmm, that is rather unclear, and looking at the code is also just
> something we don't need. These methods are doing virRaiseError,
> so we shouldn't also be doing VIR_WARN - just remove these VIR_WARN
> lines from all this code.
>

They are removed in 4/4. :)

Osier