5:19 a.m.
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