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