7:07 a.m.
This fixes several problems of shared disk management, mainly
about shared cdrom or floppy disk.
Osier Yang (4):
qemu: Add checking in helpers for sgio setting
qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk
qemu: Don't remove hash entry of other domains
qemu: Move shared disk entry adding and unpriv_sgio seting
src/qemu/qemu_conf.c | 73 ++++++++++++++++++++++++++++++--
src/qemu/qemu_conf.h | 5 +-
src/qemu/qemu_driver.c | 55 +++++++++++-------------
src/qemu/qemu_hotplug.c | 6 ++-
src/qemu/qemu_hotplug.h | 3 +-
src/qemu/qemu_migration.c | 14 +++---
src/qemu/qemu_process.c | 101 ++++++++++++++++-----------------------------
src/qemu/qemu_process.h | 6 +--
8 files changed, 149 insertions(+), 114 deletions(-)
--
1.7.7.6
7:07 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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;
+
+ 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);
@@ -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);
}
return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 30f923a..bc171a4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
{
int val = -1;
+ /* "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;
+
if (disk->sgio)
val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
@@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
_("Raw I/O is not supported on this platform"));
#endif
- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
- if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
- goto cleanup;
+ if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+ goto cleanup;
- if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
- goto cleanup;
- }
+ if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
+ goto cleanup;
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
@@ -4288,9 +4293,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
- ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
- }
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
}
/* Clear out dynamically assigned labels */
--
1.7.7.6
2:21 p.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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;
+
+ 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.
@@ -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
}
return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 30f923a..bc171a4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
{
int val = -1;
+ /* "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;
+
if (disk->sgio)
val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
@@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
_("Raw I/O is not supported on this platform"));
#endif
- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
- if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
- goto cleanup;
+ if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+ goto cleanup;
- if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
- goto cleanup;
- }
+ if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
+ goto cleanup;
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
@@ -4288,9 +4293,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
- ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
- }
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
}
/* Clear out dynamically assigned labels */
ACK - everything seems straightforward to me.
John
4:35 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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].
> @@ -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].
> }
>
> return ret;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 30f923a..bc171a4 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
> {
> int val = -1;
>
> + /* "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;
[1]
> +
> if (disk->sgio)
> val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
>
> @@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
> _("Raw I/O is not supported on this
platform"));
> #endif
>
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared)
{
> - if (qemuAddSharedDisk(driver->sharedDisks, disk->src)< 0)
> - goto cleanup;
> + if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
> + goto cleanup;
>
> - if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0)
> - goto cleanup;
> - }
> + if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0)
> + goto cleanup;
>
> if (qemuSetUnprivSGIO(disk)< 0)
> goto cleanup;
> @@ -4288,9 +4293,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> for (i = 0; i< vm->def->ndisks; i++) {
> virDomainDiskDefPtr disk = vm->def->disks[i];
>
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& disk->shared)
{
> - ignore_value(qemuRemoveSharedDisk(driver->sharedDisks,
disk->src));
> - }
> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> }
>
> /* Clear out dynamically assigned labels */
>
ACK - everything seems straightforward to me.
John
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:48 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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.
>
>>@@ -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.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:09 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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
5:14 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:19 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting
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
7:08 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk
Based on moving various checking into qemuAddSharedDisk, this
avoids the caller using it in wrong ways.
---
src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_driver.c | 5 ----
src/qemu/qemu_process.c | 53 -----------------------------------------------
src/qemu/qemu_process.h | 3 --
4 files changed, 50 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 69ba710..3eeea4b 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path)
return key;
}
+/* Check if a shared disk's setting conflicts with the conf
+ * used by other domain(s). Currently only checks the sgio
+ * setting. Note that this should only be called for disk with
+ * block source.
+ *
+ * Returns 0 if no conflicts, otherwise returns -1.
+ */
+static int
+qemuCheckSharedDisk(virHashTablePtr sharedDisks,
+ virDomainDiskDefPtr disk)
+{
+ int val;
+ size_t *ref = NULL;
+ char *key = NULL;
+ int ret = 0;
+
+ if (!(key = qemuGetSharedDiskKey(disk->src)))
+ return -1;
+
+ /* It can't be conflict if no other domain is
+ * is sharing it.
+ */
+ if (!(ref = virHashLookup(sharedDisks, key)))
+ goto cleanup;
+
+ if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
+
+ if ((val == 0 &&
+ (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
+ disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
+ (val == 1 &&
+ disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
+ goto cleanup;
+
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("sgio of shared disk '%s' conflicts with other "
+ "active domains"), disk->src);
+ ret = -1;
+
+cleanup:
+ VIR_FREE(key);
+ return ret;
+}
+
/* Increase ref count if the entry already exists, otherwise
* add a new entry.
*/
@@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
!disk->shared || !disk->src)
return 0;
+ if (qemuCheckSharedDisk(sharedDisks, disk) < 0)
+ return -1;
+
if (!(key = qemuGetSharedDiskKey(disk->src)))
return -1;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 043efd3..1dc9789 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
- disk->shared &&
- (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0))
- goto end;
-
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index bc171a4..1e0876c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
return 0;
}
-/* Check if a shared disk's setting conflicts with the conf
- * used by other domain(s). Currently only checks the sgio
- * setting. Note that this should only be called for disk with
- * block source.
- *
- * Returns 0 if no conflicts, otherwise returns -1.
- */
-int
-qemuCheckSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
-{
- int val;
- size_t *ref = NULL;
- char *key = NULL;
- int ret = 0;
-
- if (!(key = qemuGetSharedDiskKey(disk->src)))
- return -1;
-
- /* It can't be conflict if no other domain is
- * is sharing it.
- */
- if (!(ref = virHashLookup(sharedDisks, key)))
- goto cleanup;
-
- if (ref == (void *)0x1)
- goto cleanup;
-
- if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
- ret = -1;
- goto cleanup;
- }
-
- if ((val == 0 &&
- (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
- disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
- (val == 1 &&
- disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
- goto cleanup;
-
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("sgio of shared disk '%s' conflicts with other "
- "active domains"), disk->src);
- ret = -1;
-
-cleanup:
- VIR_FREE(key);
- return ret;
-}
-
int qemuProcessStart(virConnectPtr conn,
virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -3885,9 +3835,6 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
goto cleanup;
- if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
- goto cleanup;
-
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
}
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 2dc8041..7c620d4 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
virBitmapPtr nodemask);
int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);
-int qemuCheckSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk);
-
#endif /* __QEMU_PROCESS_H__ */
--
1.7.7.6
2:46 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk
On 02/08/2013 08:08 AM, Osier Yang wrote:
Based on moving various checking into qemuAddSharedDisk, this
avoids the caller using it in wrong ways.
---
src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_driver.c | 5 ----
src/qemu/qemu_process.c | 53 -----------------------------------------------
src/qemu/qemu_process.h | 3 --
4 files changed, 50 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 69ba710..3eeea4b 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path)
return key;
}
+/* Check if a shared disk's setting conflicts with the conf
+ * used by other domain(s). Currently only checks the sgio
+ * setting. Note that this should only be called for disk with
+ * block source.
+ *
+ * Returns 0 if no conflicts, otherwise returns -1.
+ */
+static int
+qemuCheckSharedDisk(virHashTablePtr sharedDisks,
+ virDomainDiskDefPtr disk)
+{
+ int val;
+ size_t *ref = NULL;
+ char *key = NULL;
+ int ret = 0;
+
+ if (!(key = qemuGetSharedDiskKey(disk->src)))
+ return -1;
+
+ /* It can't be conflict if no other domain is
+ * is sharing it.
+ */
+ if (!(ref = virHashLookup(sharedDisks, key)))
+ goto cleanup;
+
+ if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
+
+ if ((val == 0 &&
+ (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
+ disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
+ (val == 1 &&
+ disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
+ goto cleanup;
+
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("sgio of shared disk '%s' conflicts with other "
+ "active domains"), disk->src);
+ ret = -1;
+
+cleanup:
+ VIR_FREE(key);
+ return ret;
+}
+
/* Increase ref count if the entry already exists, otherwise
* add a new entry.
*/
@@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
!disk->shared || !disk->src)
return 0;
+ if (qemuCheckSharedDisk(sharedDisks, disk) < 0)
+ return -1;
+
I take the order previously in qemuProcessStart() was wrong then - it
was adding it first, then checking if it was shared. The DiskLive code
checked shared first, then added, which does seem more correct.
However, the change is subtle enough to make me wonder about the reason
for the ordering of calls in the DiskLive code that would check shared
before calls to qemuDomainDetermineDiskChain() and qemuSetupDiskCgroup()
Seems as though now the code in DiskLive may need some extra backout
code if the add now fails because of the check
if (!(key = qemuGetSharedDiskKey(disk->src)))
return -1;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 043efd3..1dc9789 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
- if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
- disk->shared &&
- (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0))
- goto end;
-
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index bc171a4..1e0876c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
return 0;
}
-/* Check if a shared disk's setting conflicts with the conf
- * used by other domain(s). Currently only checks the sgio
- * setting. Note that this should only be called for disk with
- * block source.
- *
- * Returns 0 if no conflicts, otherwise returns -1.
- */
-int
-qemuCheckSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
-{
- int val;
- size_t *ref = NULL;
- char *key = NULL;
- int ret = 0;
-
- if (!(key = qemuGetSharedDiskKey(disk->src)))
- return -1;
-
- /* It can't be conflict if no other domain is
- * is sharing it.
- */
- if (!(ref = virHashLookup(sharedDisks, key)))
- goto cleanup;
-
- if (ref == (void *)0x1)
- goto cleanup;
-
- if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
- ret = -1;
- goto cleanup;
- }
-
- if ((val == 0 &&
- (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
- disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
- (val == 1 &&
- disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
- goto cleanup;
-
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("sgio of shared disk '%s' conflicts with other "
- "active domains"), disk->src);
- ret = -1;
-
-cleanup:
- VIR_FREE(key);
- return ret;
-}
-
int qemuProcessStart(virConnectPtr conn,
virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -3885,9 +3835,6 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
goto cleanup;
- if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
- goto cleanup;
-
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
}
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 2dc8041..7c620d4 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
virBitmapPtr nodemask);
int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);
-int qemuCheckSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk);
-
#endif /* __QEMU_PROCESS_H__ */
Need to understand ramifications of change before ACK I think. Unless
someone else comes in after me with a deeper understanding and says the
order is fine.
John
4:50 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk
On 2013年02月09日 04:46, John Ferlan wrote:
On 02/08/2013 08:08 AM, Osier Yang wrote:
> Based on moving various checking into qemuAddSharedDisk, this
> avoids the caller using it in wrong ways.
> ---
> src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_driver.c | 5 ----
> src/qemu/qemu_process.c | 53 -----------------------------------------------
> src/qemu/qemu_process.h | 3 --
> 4 files changed, 50 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 69ba710..3eeea4b 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path)
> return key;
> }
>
> +/* Check if a shared disk's setting conflicts with the conf
> + * used by other domain(s). Currently only checks the sgio
> + * setting. Note that this should only be called for disk with
> + * block source.
> + *
> + * Returns 0 if no conflicts, otherwise returns -1.
> + */
> +static int
> +qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> + virDomainDiskDefPtr disk)
> +{
> + int val;
> + size_t *ref = NULL;
> + char *key = NULL;
> + int ret = 0;
> +
> + if (!(key = qemuGetSharedDiskKey(disk->src)))
> + return -1;
> +
> + /* It can't be conflict if no other domain is
> + * is sharing it.
> + */
> + if (!(ref = virHashLookup(sharedDisks, key)))
> + goto cleanup;
> +
> + if (virGetDeviceUnprivSGIO(disk->src, NULL,&val)< 0) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> + if ((val == 0&&
> + (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> + disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> + (val == 1&&
> + disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> + goto cleanup;
> +
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk '%s' conflicts with other
"
> + "active domains"), disk->src);
> + ret = -1;
> +
> +cleanup:
> + VIR_FREE(key);
> + return ret;
> +}
> +
> /* Increase ref count if the entry already exists, otherwise
> * add a new entry.
> */
> @@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
> !disk->shared || !disk->src)
> return 0;
>
> + if (qemuCheckSharedDisk(sharedDisks, disk)< 0)
> + return -1;
> +
I take the order previously in qemuProcessStart() was wrong then - it
was adding it first, then checking if it was shared. The DiskLive code
checked shared first, then added, which does seem more correct.
See PATCH 3/4, the reason for adding it first, and then checking, in
qemuProcessStart, is to avoid removing the hash entry which belongs to
other domain(s) when doing cleanup for failing on starting.
But it has flaws, because it still could removes the hash entry belongs
to other domain(s) if it fails before the block to add the hash entry
&& set unpriv_sgio.
The reason for checking first, and then adding in DiskLive is that
there is no risk to remove hash entry of other domains. But it also
has flaws for [1], which works for qemuProcessStart, but not for
DiskLive. I have ever added a new argument to the checking method
so that it knowns whether should return 0 when the "ref" count is
1 in a previous version. But it's not that nice, and I changed it
by using a bitmap for qemuProcessStop PATCH 3/4 of this version.
However, the change is subtle enough to make me wonder about the reason
for the ordering of calls in the DiskLive code that would check shared
before calls to qemuDomainDetermineDiskChain() and qemuSetupDiskCgroup()
There is no special reason, we can also put the checking after these
calls. Just I think checking first is more effective in case of
there is error when checking.
Seems as though now the code in DiskLive may need some extra backout
code if the add now fails because of the check
It's done in PATCH 4/4.
> if (!(key = qemuGetSharedDiskKey(disk->src)))
> return -1;
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 043efd3..1dc9789 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> goto end;
> }
>
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&&
> - disk->shared&&
> - (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0))
> - goto end;
> -
> if (qemuDomainDetermineDiskChain(driver, disk, false)< 0)
> goto end;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index bc171a4..1e0876c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
> return 0;
> }
>
> -/* Check if a shared disk's setting conflicts with the conf
> - * used by other domain(s). Currently only checks the sgio
> - * setting. Note that this should only be called for disk with
> - * block source.
> - *
> - * Returns 0 if no conflicts, otherwise returns -1.
> - */
> -int
> -qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> - virDomainDiskDefPtr disk)
> -{
> - int val;
> - size_t *ref = NULL;
> - char *key = NULL;
> - int ret = 0;
> -
> - if (!(key = qemuGetSharedDiskKey(disk->src)))
> - return -1;
> -
> - /* It can't be conflict if no other domain is
> - * is sharing it.
> - */
> - if (!(ref = virHashLookup(sharedDisks, key)))
> - goto cleanup;
> -
> - if (ref == (void *)0x1)
> - goto cleanup;
[1]
> -
> - if (virGetDeviceUnprivSGIO(disk->src, NULL,&val)< 0) {
> - ret = -1;
> - goto cleanup;
> - }
> -
> - if ((val == 0&&
> - (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> - disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> - (val == 1&&
> - disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> - goto cleanup;
> -
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("sgio of shared disk '%s' conflicts with other
"
> - "active domains"), disk->src);
> - ret = -1;
> -
> -cleanup:
> - VIR_FREE(key);
> - return ret;
> -}
> -
> int qemuProcessStart(virConnectPtr conn,
> virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> @@ -3885,9 +3835,6 @@ int qemuProcessStart(virConnectPtr conn,
> if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
> goto cleanup;
>
> - if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0)
> - goto cleanup;
> -
> if (qemuSetUnprivSGIO(disk)< 0)
> goto cleanup;
> }
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 2dc8041..7c620d4 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
> virBitmapPtr nodemask);
> int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);
>
> -int qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> - virDomainDiskDefPtr disk);
> -
> #endif /* __QEMU_PROCESS_H__ */
>
Need to understand ramifications of change before ACK I think. Unless
someone else comes in after me with a deeper understanding and says the
order is fine.
Thanks for the careful reviewing, the details is harry.
Osier
7:08 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).
---
src/qemu/qemu_conf.c | 5 ++++-
src/qemu/qemu_conf.h | 3 ++-
src/qemu/qemu_driver.c | 21 +++++++++++----------
src/qemu/qemu_migration.c | 14 +++++++-------
src/qemu/qemu_process.c | 37 +++++++++++++++++++++++++++++--------
src/qemu/qemu_process.h | 3 ++-
6 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3eeea4b..a6162b6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -773,7 +773,8 @@ cleanup:
*/
int
qemuAddSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int *added)
{
size_t *ref = NULL;
char *key = NULL;
@@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
}
}
+ if (added)
+ *added = 1;
VIR_FREE(key);
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 8e84bcf..b8b5275 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virConnectPtr conn);
int qemuAddSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int *added)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1dc9789..03fe526 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
goto endjob;
}
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
@@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
goto endjob;
/* Shut it down */
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
virDomainAuditStop(vm, "saved");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
endjob:
if ((ret == 0) && (flags & VIR_DUMP_CRASH)) {
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
virDomainAuditStop(vm, "crashed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
}
if (virCommandWait(cmd, NULL) < 0) {
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
ret = -1;
}
VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
@@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
}
if (ret == 0) {
- if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
VIR_WARN("Failed to add disk '%s' to shared disk table",
disk->src);
@@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
/* We already filtered the _HALT flag for persistent domains
* only, so this end job never drops the last reference. */
@@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
/* We already filtered the _HALT flag for persistent domains
* only, so this end job never drops the last reference. */
@@ -12151,8 +12151,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
goto endjob;
}
virResetError(err);
- qemuProcessStop(driver, vm,
- VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
event = virDomainEventNewFromObj(vm,
@@ -12265,7 +12265,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
if (virDomainObjIsActive(vm)) {
/* Transitions 4, 7 */
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
event = virDomainEventNewFromObj(vm,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a75fb4e..365afe3 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1692,7 +1692,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
virReportSystemError(errno, "%s",
_("cannot pass pipe for tunnelled
migration"));
virDomainAuditStart(vm, "migrated", false);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
goto endjob;
}
dataFD[1] = -1; /* 'st' owns the FD now & will close it */
@@ -3057,7 +3057,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
*/
if (!v3proto) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "migrated");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3359,7 +3359,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
if (!(flags & VIR_MIGRATE_OFFLINE)) {
if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3398,7 +3398,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
if (v3proto) {
if (!(flags & VIR_MIGRATE_OFFLINE)) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
}
if (newVM)
@@ -3446,7 +3446,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
*/
if (v3proto) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3483,7 +3483,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
qemuProcessAutoDestroyRemove(driver, vm);
} else if (!(flags & VIR_MIGRATE_OFFLINE)) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3554,7 +3554,7 @@ int qemuMigrationConfirm(virQEMUDriverPtr driver,
*/
if (retcode == 0) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "migrated");
event = virDomainEventNewFromObj(vm,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1e0876c..f820c57 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -333,7 +333,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
eventReason);
- qemuProcessStop(driver, vm, stopReason, 0);
+ qemuProcessStop(driver, vm, stopReason, 0, NULL);
virDomainAuditStop(vm, auditReason);
if (!vm->persistent) {
@@ -3340,7 +3340,7 @@ error:
* really is and FAILED means "failed to start" */
state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
}
- qemuProcessStop(driver, obj, state, 0);
+ qemuProcessStop(driver, obj, state, 0, NULL);
if (!obj->persistent)
qemuDomainRemoveInactive(driver, obj);
else
@@ -3417,7 +3417,8 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
} else if (virObjectUnref(obj)) {
/* We can't spawn a thread and thus connect to monitor.
* Kill qemu */
- qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+ qemuProcessStop(src->driver, obj,
+ VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
if (!obj->persistent)
qemuDomainRemoveInactive(src->driver, obj);
else
@@ -3507,6 +3508,7 @@ int qemuProcessStart(virConnectPtr conn,
virBitmapPtr nodemask = NULL;
unsigned int stop_flags;
virQEMUDriverConfigPtr cfg;
+ virBitmapPtr added_disks = NULL;
/* Okay, these are just internal flags,
* but doesn't hurt to check */
@@ -3820,9 +3822,15 @@ int qemuProcessStart(virConnectPtr conn,
if (cfg->clearEmulatorCapabilities)
virCommandClearCaps(cmd);
+ if (!(added_disks = virBitmapNew(vm->def->ndisks))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
+ int added;
if (vm->def->disks[i]->rawio == 1)
#ifdef CAP_SYS_RAWIO
@@ -3832,8 +3840,10 @@ int qemuProcessStart(virConnectPtr conn,
_("Raw I/O is not supported on this platform"));
#endif
- if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
goto cleanup;
+ if (added)
+ ignore_value(virBitmapSetBit(added_disks, i));
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
@@ -4049,6 +4059,7 @@ int qemuProcessStart(virConnectPtr conn,
}
virCommandFree(cmd);
+ virBitmapFree(added_disks);
VIR_FORCE_CLOSE(logfile);
virObjectUnref(cfg);
@@ -4062,7 +4073,8 @@ cleanup:
virBitmapFree(nodemask);
virCommandFree(cmd);
VIR_FORCE_CLOSE(logfile);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
+ stop_flags, added_disks);
virObjectUnref(cfg);
return -1;
@@ -4113,7 +4125,8 @@ qemuProcessKill(virQEMUDriverPtr driver,
void qemuProcessStop(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainShutoffReason reason,
- unsigned int flags)
+ unsigned int flags,
+ virBitmapPtr added_disks)
{
int ret;
int retries = 0;
@@ -4239,9 +4252,17 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
+ bool is_added;
- ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+ if (added_disks) {
+ ignore_value(virBitmapGetBit(added_disks, i, &is_added));
+ if (is_added)
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+ } else {
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+ }
}
+ virBitmapFree(added_disks);
/* Clear out dynamically assigned labels */
for (i = 0; i < vm->def->nseclabels; i++) {
@@ -4594,7 +4615,7 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver,
VIR_DEBUG("Killing domain");
qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(dom, "destroyed");
event = virDomainEventNewFromObj(dom,
VIR_DOMAIN_EVENT_STOPPED,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 7c620d4..c4f8a6a 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -68,7 +68,8 @@ typedef enum {
void qemuProcessStop(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainShutoffReason reason,
- unsigned int flags);
+ unsigned int flags,
+ virBitmapPtr added_disks);
int qemuProcessAttach(virConnectPtr conn,
virQEMUDriverPtr driver,
--
1.7.7.6
3:09 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On 02/08/2013 08:08 AM, Osier Yang wrote:
qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).
---
src/qemu/qemu_conf.c | 5 ++++-
src/qemu/qemu_conf.h | 3 ++-
src/qemu/qemu_driver.c | 21 +++++++++++----------
src/qemu/qemu_migration.c | 14 +++++++-------
src/qemu/qemu_process.c | 37 +++++++++++++++++++++++++++++--------
src/qemu/qemu_process.h | 3 ++-
6 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3eeea4b..a6162b6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -773,7 +773,8 @@ cleanup:
*/
int
qemuAddSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int *added)
{
size_t *ref = NULL;
char *key = NULL;
@@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
}
}
+ if (added)
+ *added = 1;
VIR_FREE(key);
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 8e84bcf..b8b5275 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
virConnectPtr conn);
int qemuAddSharedDisk(virHashTablePtr sharedDisks,
- virDomainDiskDefPtr disk)
+ virDomainDiskDefPtr disk,
+ int *added)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1dc9789..03fe526 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
goto endjob;
}
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
@@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
goto endjob;
/* Shut it down */
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
virDomainAuditStop(vm, "saved");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
endjob:
if ((ret == 0) && (flags & VIR_DUMP_CRASH)) {
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
virDomainAuditStop(vm, "crashed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
}
if (virCommandWait(cmd, NULL) < 0) {
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
ret = -1;
}
VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
@@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
}
if (ret == 0) {
- if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
VIR_WARN("Failed to add disk '%s' to shared disk table",
disk->src);
@@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
/* We already filtered the _HALT flag for persistent domains
* only, so this end job never drops the last reference. */
@@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
/* We already filtered the _HALT flag for persistent domains
* only, so this end job never drops the last reference. */
@@ -12151,8 +12151,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
goto endjob;
}
virResetError(err);
- qemuProcessStop(driver, vm,
- VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
event = virDomainEventNewFromObj(vm,
@@ -12265,7 +12265,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
if (virDomainObjIsActive(vm)) {
/* Transitions 4, 7 */
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+ 0, NULL);
virDomainAuditStop(vm, "from-snapshot");
detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
event = virDomainEventNewFromObj(vm,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a75fb4e..365afe3 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1692,7 +1692,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
virReportSystemError(errno, "%s",
_("cannot pass pipe for tunnelled
migration"));
virDomainAuditStart(vm, "migrated", false);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
goto endjob;
}
dataFD[1] = -1; /* 'st' owns the FD now & will close it */
@@ -3057,7 +3057,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
*/
if (!v3proto) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "migrated");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3359,7 +3359,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
if (!(flags & VIR_MIGRATE_OFFLINE)) {
if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3398,7 +3398,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
if (v3proto) {
if (!(flags & VIR_MIGRATE_OFFLINE)) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
}
if (newVM)
@@ -3446,7 +3446,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
*/
if (v3proto) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3483,7 +3483,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
qemuProcessAutoDestroyRemove(driver, vm);
} else if (!(flags & VIR_MIGRATE_OFFLINE)) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "failed");
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
@@ -3554,7 +3554,7 @@ int qemuMigrationConfirm(virQEMUDriverPtr driver,
*/
if (retcode == 0) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(vm, "migrated");
event = virDomainEventNewFromObj(vm,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1e0876c..f820c57 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -333,7 +333,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,
eventReason);
- qemuProcessStop(driver, vm, stopReason, 0);
+ qemuProcessStop(driver, vm, stopReason, 0, NULL);
virDomainAuditStop(vm, auditReason);
if (!vm->persistent) {
@@ -3340,7 +3340,7 @@ error:
* really is and FAILED means "failed to start" */
state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
}
- qemuProcessStop(driver, obj, state, 0);
+ qemuProcessStop(driver, obj, state, 0, NULL);
if (!obj->persistent)
qemuDomainRemoveInactive(driver, obj);
else
@@ -3417,7 +3417,8 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
} else if (virObjectUnref(obj)) {
/* We can't spawn a thread and thus connect to monitor.
* Kill qemu */
- qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+ qemuProcessStop(src->driver, obj,
+ VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
if (!obj->persistent)
qemuDomainRemoveInactive(src->driver, obj);
else
@@ -3507,6 +3508,7 @@ int qemuProcessStart(virConnectPtr conn,
virBitmapPtr nodemask = NULL;
unsigned int stop_flags;
virQEMUDriverConfigPtr cfg;
+ virBitmapPtr added_disks = NULL;
/* Okay, these are just internal flags,
* but doesn't hurt to check */
@@ -3820,9 +3822,15 @@ int qemuProcessStart(virConnectPtr conn,
if (cfg->clearEmulatorCapabilities)
virCommandClearCaps(cmd);
+ if (!(added_disks = virBitmapNew(vm->def->ndisks))) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
/* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
+ int added;
if (vm->def->disks[i]->rawio == 1)
#ifdef CAP_SYS_RAWIO
@@ -3832,8 +3840,10 @@ int qemuProcessStart(virConnectPtr conn,
_("Raw I/O is not supported on this platform"));
#endif
- if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
goto cleanup;
+ if (added)
+ ignore_value(virBitmapSetBit(added_disks, i));
if (qemuSetUnprivSGIO(disk) < 0)
goto cleanup;
@@ -4049,6 +4059,7 @@ int qemuProcessStart(virConnectPtr conn,
}
virCommandFree(cmd);
+ virBitmapFree(added_disks);
VIR_FORCE_CLOSE(logfile);
virObjectUnref(cfg);
@@ -4062,7 +4073,8 @@ cleanup:
virBitmapFree(nodemask);
virCommandFree(cmd);
VIR_FORCE_CLOSE(logfile);
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
+ stop_flags, added_disks);
Do your virBitmapFree(added_disks) here not in called function...
virObjectUnref(cfg);
return -1;
@@ -4113,7 +4125,8 @@ qemuProcessKill(virQEMUDriverPtr driver,
void qemuProcessStop(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainShutoffReason reason,
- unsigned int flags)
+ unsigned int flags,
+ virBitmapPtr added_disks)
{
int ret;
int retries = 0;
@@ -4239,9 +4252,17 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ndisks; i++) {
virDomainDiskDefPtr disk = vm->def->disks[i];
+ bool is_added;
Initialize to false, since result can be unchanged if :
int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result)
{
if (bitmap->max_bit <= b)
return -1;
- ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+ if (added_disks) {
+ ignore_value(virBitmapGetBit(added_disks, i, &is_added));
+ if (is_added)
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+ } else {
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+ }
}
+ virBitmapFree(added_disks);
Let the caller do this. Think of it this way, if you make this call -
the local added_disks ends up NULL, but that is not reflected back in
the caller since this was passed by value and not reference. For some
this is a coding style - caller did the alloc and it should do the free.
/* Clear out dynamically assigned labels */
for (i = 0; i < vm->def->nseclabels; i++) {
@@ -4594,7 +4615,7 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver,
VIR_DEBUG("Killing domain");
qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED,
- VIR_QEMU_PROCESS_STOP_MIGRATED);
+ VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
virDomainAuditStop(dom, "destroyed");
event = virDomainEventNewFromObj(dom,
VIR_DOMAIN_EVENT_STOPPED,
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 7c620d4..c4f8a6a 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -68,7 +68,8 @@ typedef enum {
void qemuProcessStop(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainShutoffReason reason,
- unsigned int flags);
+ unsigned int flags,
+ virBitmapPtr added_disks);
int qemuProcessAttach(virConnectPtr conn,
virQEMUDriverPtr driver,
This is quite a bit of churn for the one place you needed the bitmap.
Would there be need/cause to add a field to the driver struct with the
shared disk bitmap rather than a parameter to qemuProcessStop(). Perhaps
there'd be other uses for it too (just thinking out loud late on snowy
Friday afternoon).
John
5:22 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On 2013年02月09日 05:09, John Ferlan wrote:
On 02/08/2013 08:08 AM, Osier Yang wrote:
> qemuProcessStart invokes qemuProcessStop when fails, to avoid
> removing hash entry which belongs to other domain(s), this introduces
> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
> sets the bit for the disk only if it's successfully added into the
> hash table. Thus if the argument is provided for qemuProcessStop, it
> can't remove the hash entry belongs to other domain(s).
> ---
> src/qemu/qemu_conf.c | 5 ++++-
> src/qemu/qemu_conf.h | 3 ++-
> src/qemu/qemu_driver.c | 21 +++++++++++----------
> src/qemu/qemu_migration.c | 14 +++++++-------
> src/qemu/qemu_process.c | 37 +++++++++++++++++++++++++++++--------
> src/qemu/qemu_process.h | 3 ++-
> 6 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 3eeea4b..a6162b6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -773,7 +773,8 @@ cleanup:
> */
> int
> qemuAddSharedDisk(virHashTablePtr sharedDisks,
> - virDomainDiskDefPtr disk)
> + virDomainDiskDefPtr disk,
> + int *added)
> {
> size_t *ref = NULL;
> char *key = NULL;
> @@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
> }
> }
>
> + if (added)
> + *added = 1;
> VIR_FREE(key);
> return 0;
> }
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 8e84bcf..b8b5275 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
> virConnectPtr conn);
>
> int qemuAddSharedDisk(virHashTablePtr sharedDisks,
> - virDomainDiskDefPtr disk)
> + virDomainDiskDefPtr disk,
> + int *added)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1dc9789..03fe526 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
> goto endjob;
> }
>
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
> @@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr
dom,
> goto endjob;
>
> /* Shut it down */
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
> virDomainAuditStop(vm, "saved");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
>
> endjob:
> if ((ret == 0)&& (flags& VIR_DUMP_CRASH)) {
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
> virDomainAuditStop(vm, "crashed");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> }
>
> if (virCommandWait(cmd, NULL)< 0) {
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
> ret = -1;
> }
> VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
> @@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> }
>
> if (ret == 0) {
> - if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
> + if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL)< 0)
> VIR_WARN("Failed to add disk '%s' to shared disk
table",
> disk->src);
>
> @@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
> if (flags& VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
> virDomainAuditStop(vm, "from-snapshot");
> /* We already filtered the _HALT flag for persistent domains
> * only, so this end job never drops the last reference. */
> @@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>
> event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
> virDomainAuditStop(vm, "from-snapshot");
> /* We already filtered the _HALT flag for persistent domains
> * only, so this end job never drops the last reference. */
> @@ -12151,8 +12151,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
> goto endjob;
> }
> virResetError(err);
> - qemuProcessStop(driver, vm,
> - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
> + 0, NULL);
> virDomainAuditStop(vm, "from-snapshot");
> detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
> event = virDomainEventNewFromObj(vm,
> @@ -12265,7 +12265,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>
> if (virDomainObjIsActive(vm)) {
> /* Transitions 4, 7 */
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
> + 0, NULL);
> virDomainAuditStop(vm, "from-snapshot");
> detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
> event = virDomainEventNewFromObj(vm,
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index a75fb4e..365afe3 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1692,7 +1692,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> virReportSystemError(errno, "%s",
> _("cannot pass pipe for tunnelled
migration"));
> virDomainAuditStart(vm, "migrated", false);
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
> goto endjob;
> }
> dataFD[1] = -1; /* 'st' owns the FD now& will close it */
> @@ -3057,7 +3057,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
> */
> if (!v3proto) {
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(vm, "migrated");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -3359,7 +3359,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> if (!(flags& VIR_MIGRATE_OFFLINE)) {
> if (qemuMigrationVPAssociatePortProfiles(vm->def)< 0) {
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(vm, "failed");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -3398,7 +3398,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> if (v3proto) {
> if (!(flags& VIR_MIGRATE_OFFLINE)) {
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(vm, "failed");
> }
> if (newVM)
> @@ -3446,7 +3446,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> */
> if (v3proto) {
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(vm, "failed");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -3483,7 +3483,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> qemuProcessAutoDestroyRemove(driver, vm);
> } else if (!(flags& VIR_MIGRATE_OFFLINE)) {
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(vm, "failed");
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> @@ -3554,7 +3554,7 @@ int qemuMigrationConfirm(virQEMUDriverPtr driver,
> */
> if (retcode == 0) {
> qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(vm, "migrated");
>
> event = virDomainEventNewFromObj(vm,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1e0876c..f820c57 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -333,7 +333,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> event = virDomainEventNewFromObj(vm,
> VIR_DOMAIN_EVENT_STOPPED,
> eventReason);
> - qemuProcessStop(driver, vm, stopReason, 0);
> + qemuProcessStop(driver, vm, stopReason, 0, NULL);
> virDomainAuditStop(vm, auditReason);
>
> if (!vm->persistent) {
> @@ -3340,7 +3340,7 @@ error:
> * really is and FAILED means "failed to start" */
> state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
> }
> - qemuProcessStop(driver, obj, state, 0);
> + qemuProcessStop(driver, obj, state, 0, NULL);
> if (!obj->persistent)
> qemuDomainRemoveInactive(driver, obj);
> else
> @@ -3417,7 +3417,8 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
> } else if (virObjectUnref(obj)) {
> /* We can't spawn a thread and thus connect to monitor.
> * Kill qemu */
> - qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> + qemuProcessStop(src->driver, obj,
> + VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
> if (!obj->persistent)
> qemuDomainRemoveInactive(src->driver, obj);
> else
> @@ -3507,6 +3508,7 @@ int qemuProcessStart(virConnectPtr conn,
> virBitmapPtr nodemask = NULL;
> unsigned int stop_flags;
> virQEMUDriverConfigPtr cfg;
> + virBitmapPtr added_disks = NULL;
>
> /* Okay, these are just internal flags,
> * but doesn't hurt to check */
> @@ -3820,9 +3822,15 @@ int qemuProcessStart(virConnectPtr conn,
> if (cfg->clearEmulatorCapabilities)
> virCommandClearCaps(cmd);
>
> + if (!(added_disks = virBitmapNew(vm->def->ndisks))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
> for (i = 0; i< vm->def->ndisks; i++) {
> virDomainDiskDefPtr disk = vm->def->disks[i];
> + int added;
>
> if (vm->def->disks[i]->rawio == 1)
> #ifdef CAP_SYS_RAWIO
> @@ -3832,8 +3840,10 @@ int qemuProcessStart(virConnectPtr conn,
> _("Raw I/O is not supported on this
platform"));
> #endif
>
> - if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0)
> + if (qemuAddSharedDisk(driver->sharedDisks, disk,&added)< 0)
> goto cleanup;
> + if (added)
> + ignore_value(virBitmapSetBit(added_disks, i));
>
> if (qemuSetUnprivSGIO(disk)< 0)
> goto cleanup;
> @@ -4049,6 +4059,7 @@ int qemuProcessStart(virConnectPtr conn,
> }
>
> virCommandFree(cmd);
> + virBitmapFree(added_disks);
> VIR_FORCE_CLOSE(logfile);
> virObjectUnref(cfg);
>
> @@ -4062,7 +4073,8 @@ cleanup:
> virBitmapFree(nodemask);
> virCommandFree(cmd);
> VIR_FORCE_CLOSE(logfile);
> - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
> + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> + stop_flags, added_disks);
Do your virBitmapFree(added_disks) here not in called function...
Agreed.
> virObjectUnref(cfg);
>
> return -1;
> @@ -4113,7 +4125,8 @@ qemuProcessKill(virQEMUDriverPtr driver,
> void qemuProcessStop(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainShutoffReason reason,
> - unsigned int flags)
> + unsigned int flags,
> + virBitmapPtr added_disks)
> {
> int ret;
> int retries = 0;
> @@ -4239,9 +4252,17 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>
> for (i = 0; i< vm->def->ndisks; i++) {
> virDomainDiskDefPtr disk = vm->def->disks[i];
> + bool is_added;
Initialize to false, since result can be unchanged if :
int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result)
{
if (bitmap->max_bit<= b)
return -1;
Okay.
>
> - ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> + if (added_disks) {
> + ignore_value(virBitmapGetBit(added_disks, i,&is_added));
> + if (is_added)
> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> + } else {
> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> + }
> }
> + virBitmapFree(added_disks);
Let the caller do this. Think of it this way, if you make this call -
the local added_disks ends up NULL, but that is not reflected back in
the caller since this was passed by value and not reference. For some
this is a coding style - caller did the alloc and it should do the free.
Agreed, freeing after call is more clear.
>
> /* Clear out dynamically assigned labels */
> for (i = 0; i< vm->def->nseclabels; i++) {
> @@ -4594,7 +4615,7 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver,
>
> VIR_DEBUG("Killing domain");
> qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED,
> - VIR_QEMU_PROCESS_STOP_MIGRATED);
> + VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
> virDomainAuditStop(dom, "destroyed");
> event = virDomainEventNewFromObj(dom,
> VIR_DOMAIN_EVENT_STOPPED,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 7c620d4..c4f8a6a 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -68,7 +68,8 @@ typedef enum {
> void qemuProcessStop(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainShutoffReason reason,
> - unsigned int flags);
> + unsigned int flags,
> + virBitmapPtr added_disks);
>
> int qemuProcessAttach(virConnectPtr conn,
> virQEMUDriverPtr driver,
>
This is quite a bit of churn for the one place you needed the bitmap.
Would there be need/cause to add a field to the driver struct with the
shared disk bitmap rather than a parameter to qemuProcessStop(). Perhaps
there'd be other uses for it too (just thinking out loud late on snowy
Friday afternoon).
Don't agree. As it's only used by qemuProcessStart. And it's per-VM
basis, not worth to maintain it in the driver life cycle.
Osier
4:59 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:
qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).
I find this approach rather dubious - IMHO it is a sign that you're
not recording enough information in the shared disk hash. eg perhaps
the hash ought to be recording the UUID of each VM that is holding
a reference. That way you're protected from qemuProcessStop() trying
todo something wrong.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:37 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On 2013年02月11日 18:59, Daniel P. Berrange wrote:
On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:
> qemuProcessStart invokes qemuProcessStop when fails, to avoid
> removing hash entry which belongs to other domain(s), this introduces
> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
> sets the bit for the disk only if it's successfully added into the
> hash table. Thus if the argument is provided for qemuProcessStop, it
> can't remove the hash entry belongs to other domain(s).
I find this approach rather dubious - IMHO it is a sign that you're
not recording enough information in the shared disk hash. eg perhaps
the hash ought to be recording the UUID of each VM that is holding
a reference. That way you're protected from qemuProcessStop() trying
todo something wrong.
I'm doubting about if it's really deserved to maintain a bunch of
arrays in the hash entry. As we only need the recording for
qemuProcessStart, it's much simpler to only use a bitmap to record
the added disks for a VM in qemuProcessStart from my p.o.v.
Osier
11:44 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On Mon, Feb 11, 2013 at 07:37:14PM +0800, Osier Yang wrote:
On 2013年02月11日 18:59, Daniel P. Berrange wrote:
>On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:
>>qemuProcessStart invokes qemuProcessStop when fails, to avoid
>>removing hash entry which belongs to other domain(s), this introduces
>>a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
>>sets the bit for the disk only if it's successfully added into the
>>hash table. Thus if the argument is provided for qemuProcessStop, it
>>can't remove the hash entry belongs to other domain(s).
>
>I find this approach rather dubious - IMHO it is a sign that you're
>not recording enough information in the shared disk hash. eg perhaps
>the hash ought to be recording the UUID of each VM that is holding
>a reference. That way you're protected from qemuProcessStop() trying
>todo something wrong.
>
I'm doubting about if it's really deserved to maintain a bunch of
arrays in the hash entry. As we only need the recording for
qemuProcessStart, it's much simpler to only use a bitmap to record
the added disks for a VM in qemuProcessStart from my p.o.v.
IMHO it is a more robust design to record which VM is owning the
disk, since it prevents us introducing errors elsewhere in the
code which can lead to the same kind of problem later.
Incidentally, how does the shared disks hash get populated when
libvirtd starts up & reconnects to existing running VMs ? AFAICT,
nothing is being done in that codepath.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
1:09 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains
On 2013年02月12日 01:44, Daniel P. Berrange wrote:
On Mon, Feb 11, 2013 at 07:37:14PM +0800, Osier Yang wrote:
> On 2013年02月11日 18:59, Daniel P. Berrange wrote:
>> On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:
>>> qemuProcessStart invokes qemuProcessStop when fails, to avoid
>>> removing hash entry which belongs to other domain(s), this introduces
>>> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
>>> sets the bit for the disk only if it's successfully added into the
>>> hash table. Thus if the argument is provided for qemuProcessStop, it
>>> can't remove the hash entry belongs to other domain(s).
>>
>> I find this approach rather dubious - IMHO it is a sign that you're
>> not recording enough information in the shared disk hash. eg perhaps
>> the hash ought to be recording the UUID of each VM that is holding
>> a reference. That way you're protected from qemuProcessStop() trying
>> todo something wrong.
>>
>
> I'm doubting about if it's really deserved to maintain a bunch of
> arrays in the hash entry. As we only need the recording for
> qemuProcessStart, it's much simpler to only use a bitmap to record
> the added disks for a VM in qemuProcessStart from my p.o.v.
IMHO it is a more robust design to record which VM is owning the
disk, since it prevents us introducing errors elsewhere in the
code which can lead to the same kind of problem later.
Okay, I have no more reason to not go this way.
Incidentally, how does the shared disks hash get populated when
libvirtd starts up& reconnects to existing running VMs ? AFAICT,
nothing is being done in that codepath.
Indeed, will add.
Daniel
7:08 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting
The disk def could be free'ed by qemuDomainChangeEjectableMedia
for cdrom or floppy disk. This moves the adding and setting before
the attaching takes place. And for cdrom floppy disk, if the
change is ejecting, removing the existed hash entry for it.
---
src/qemu/qemu_driver.c | 23 +++++++++++++----------
src/qemu/qemu_hotplug.c | 6 +++++-
src/qemu/qemu_hotplug.h | 3 ++-
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 03fe526..4aad42f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
{
virDomainDiskDefPtr disk = dev->data.disk;
virCgroupPtr cgroup = NULL;
+ int eject, added;
int ret = -1;
if (disk->driverName != NULL && !STREQ(disk->driverName,
"qemu")) {
@@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
+ goto end;
+
+ if (qemuSetUnprivSGIO(disk) < 0)
+ goto end;
+
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
@@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
switch (disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
+ ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, &eject);
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
case VIR_DOMAIN_DISK_DEVICE_LUN:
@@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
break;
}
+ if (ret == 0 && eject)
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+
if (ret != 0 && cgroup) {
if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
VIR_WARN("Failed to teardown cgroup for disk path %s",
NULLSTR(disk->src));
}
- if (ret == 0) {
- if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 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);
- }
-
end:
+ if (ret != 0 && added)
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk))
if (cgroup)
virCgroupFree(&cgroup);
return ret;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 98912bf..18aca47 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -53,7 +53,8 @@
int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- bool force)
+ bool force,
+ int *eject)
{
virDomainDiskDefPtr origdisk = NULL;
int i;
@@ -93,6 +94,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
goto cleanup;
}
+ if (eject && origdisk->src && !disk->src)
+ *eject = 1;
+
if (virDomainLockDiskAttach(driver->lockManager, cfg->uri,
vm, disk) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 8f01d23..fc0532a 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -31,7 +31,8 @@
int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- bool force);
+ bool force,
+ int *eject);
int qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
enum qemuDomainAsyncJob asyncJob);
--
1.7.7.6
3:32 p.m.
New subject: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting
On 02/08/2013 08:08 AM, Osier Yang wrote:
The disk def could be free'ed by qemuDomainChangeEjectableMedia
for cdrom or floppy disk. This moves the adding and setting before
the attaching takes place. And for cdrom floppy disk, if the
change is ejecting, removing the existed hash entry for it.
---
src/qemu/qemu_driver.c | 23 +++++++++++++----------
src/qemu/qemu_hotplug.c | 6 +++++-
src/qemu/qemu_hotplug.h | 3 ++-
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 03fe526..4aad42f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
{
virDomainDiskDefPtr disk = dev->data.disk;
virCgroupPtr cgroup = NULL;
+ int eject, added;
Initialize eject and added
int ret = -1;
if (disk->driverName != NULL && !STREQ(disk->driverName,
"qemu")) {
@@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
+ goto end;
+
+ if (qemuSetUnprivSGIO(disk) < 0)
+ goto end;
+
hrmph. makes my comment in 2/4 less important now. Although it does
make me think that qemuSetUnprivSGIO() could be at the tail end of
qemuAddSharedDisk() since both here and qemuProcessStart() call it.
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
@@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
switch (disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
+ ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, &eject);
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
case VIR_DOMAIN_DISK_DEVICE_LUN:
@@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
break;
}
+ if (ret == 0 && eject)
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
+
'ret' could be 0 here, but eject undefined... Also we're going to add it
just to remove it?
if (ret != 0 && cgroup) {
if (qemuTeardownDiskCgroup(vm, cgroup, disk) < 0)
VIR_WARN("Failed to teardown cgroup for disk path %s",
NULLSTR(disk->src));
}
- if (ret == 0) {
- if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 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);
- }
-
Rather than move this up to the top, why not swap it with the cgroup
teardown and set ret = qemuAddSharedDisk(); Then the 'eject' code is
perhaps unnecessary. It seems it only exists because you added an
ejectable media to the shared device list and now need to remove it.
end:
+ if (ret != 0 && added)
You can get here from "if (disk->driverName != NULL &&
!STREQ(disk->driverName, "qemu"))" without added being set and ret ==
-1
Of course if you take my suggestion to swap cgroup teardown, then this
becomes moot.
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks,
disk))
if (cgroup)
virCgroupFree(&cgroup);
return ret;
There are two callers to qemuDomainChangeEjectableMedia() in
qemu_driver.c - you only changed one (unless I forgot/missed a prior
patch removing it!).
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 98912bf..18aca47 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -53,7 +53,8 @@
int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- bool force)
+ bool force,
+ int *eject)
{
virDomainDiskDefPtr origdisk = NULL;
int i;
@@ -93,6 +94,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
goto cleanup;
}
+ if (eject && origdisk->src && !disk->src)
+ *eject = 1;
+
if (virDomainLockDiskAttach(driver->lockManager, cfg->uri,
vm, disk) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index 8f01d23..fc0532a 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -31,7 +31,8 @@
int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainDiskDefPtr disk,
- bool force);
+ bool force,
+ int *eject);
int qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
virDomainObjPtr vm,
enum qemuDomainAsyncJob asyncJob);
John
4:55 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting
On Fri, Feb 08, 2013 at 09:08:02PM +0800, Osier Yang wrote:
The disk def could be free'ed by qemuDomainChangeEjectableMedia
for cdrom or floppy disk. This moves the adding and setting before
the attaching takes place. And for cdrom floppy disk, if the
change is ejecting, removing the existed hash entry for it.
---
src/qemu/qemu_driver.c | 23 +++++++++++++----------
src/qemu/qemu_hotplug.c | 6 +++++-
src/qemu/qemu_hotplug.h | 3 ++-
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 03fe526..4aad42f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
{
virDomainDiskDefPtr disk = dev->data.disk;
virCgroupPtr cgroup = NULL;
+ int eject, added;
int ret = -1;
if (disk->driverName != NULL && !STREQ(disk->driverName,
"qemu")) {
@@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
goto end;
}
+ if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
+ goto end;
+
+ if (qemuSetUnprivSGIO(disk) < 0)
+ goto end;
+
if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
goto end;
@@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
switch (disk->device) {
case VIR_DOMAIN_DISK_DEVICE_CDROM:
case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
- ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
+ ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, &eject);
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
case VIR_DOMAIN_DISK_DEVICE_LUN:
@@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
break;
}
+ if (ret == 0 && eject)
+ ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
This doesn't make sense - you're removing the disk we just added.
You need to remove the *old* disk->src surely ? In addition it is
*not* valid to reference 'disk' at all at this point, since the
functions we just called may have free'd it.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:30 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting
On 2013年02月11日 18:55, Daniel P. Berrange wrote:
On Fri, Feb 08, 2013 at 09:08:02PM +0800, Osier Yang wrote:
> The disk def could be free'ed by qemuDomainChangeEjectableMedia
> for cdrom or floppy disk. This moves the adding and setting before
> the attaching takes place. And for cdrom floppy disk, if the
> change is ejecting, removing the existed hash entry for it.
> ---
> src/qemu/qemu_driver.c | 23 +++++++++++++----------
> src/qemu/qemu_hotplug.c | 6 +++++-
> src/qemu/qemu_hotplug.h | 3 ++-
> 3 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 03fe526..4aad42f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> {
> virDomainDiskDefPtr disk = dev->data.disk;
> virCgroupPtr cgroup = NULL;
> + int eject, added;
> int ret = -1;
>
> if (disk->driverName != NULL&& !STREQ(disk->driverName,
"qemu")) {
> @@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> goto end;
> }
>
> + if (qemuAddSharedDisk(driver->sharedDisks, disk,&added)< 0)
> + goto end;
> +
> + if (qemuSetUnprivSGIO(disk)< 0)
> + goto end;
> +
> if (qemuDomainDetermineDiskChain(driver, disk, false)< 0)
> goto end;
>
> @@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> switch (disk->device) {
> case VIR_DOMAIN_DISK_DEVICE_CDROM:
> case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
> - ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
> + ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false,&eject);
> break;
> case VIR_DOMAIN_DISK_DEVICE_DISK:
> case VIR_DOMAIN_DISK_DEVICE_LUN:
> @@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> break;
> }
>
> + if (ret == 0&& eject)
> + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
This doesn't make sense - you're removing the disk we just added.
No, the removing only happens when the operation is to eject the
media of CD-ROM or Floppy. It's determined by "eject".
You need to remove the *old* disk->src surely ?
Yes, if the operation is ejecting.
In addition it is
*not* valid to reference 'disk' at all at this point, since the
functions we just called may have free'd it.
Oh, yes, I should copy the disk def before
qemuDomainChangeEjectableMedia takes place.
Osier
4303
days inactive
4306
days old
21 comments
3 participants
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Osier Yang