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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
