4:35 a.m.
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