1:15 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1201143
Patch 1 is just a doc change - more a clarification regarding the 'exact'
configuration for https://bugzilla.redhat.com/show_bug.cgi?id=1228007
Patch 2 adds a configuration time check for the correct settings when
someone tries to configure a lun. This does to a degree duplicate some
checks in the run time check qemuCheckDiskConfig, but since allowing the
configuration was causing issues for at least virt-manager, it was
requested to do config time checks. The copious comment in the code
notes that the volume check is only partial since at the time of
post parse device callback, the Translation of source pool data to
disk data has not occurred, so the "best" that can be done is to check
if at least a non direct 'lun' volume type is being used (unless of
course we translated the source pool).
John Ferlan (2):
docs: Clarification for when allowed to use 'lun' for "volume"
conf: Validate disk lun using correct types
docs/formatdomain.html.in | 3 ++-
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 +-
3 files changed, 25 insertions(+), 2 deletions(-)
--
2.1.0
1:15 p.m.
New subject: [libvirt] [PATCH 1/2] docs: Clarification for when allowed to use 'lun' for "volume"
While re-reading what I wrote for commit id '785a8940e', I realized
I needed to clarify that being able to present as a 'lun', the mode
property for the pool source element needed to be "host" (or empty)
and not "direct".
It was described correctly later in the mode host description, but
this just ensures it's not missed here as well.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
docs/formatdomain.html.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 172b84a..6c3459e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1972,7 +1972,8 @@
Using "lun" (<span class="since">since
0.9.10</span>) is only
valid when the <code>type</code> is "block" or
"network" for
<code>protocol='iscsi'</code> or when the
<code>type</code>
- is "volume" when using an iSCSI source
<code>pool</code>.
+ is "volume" when using an iSCSI source
<code>pool</code>
+ for <code>mode</code> "host".
Configured in this manner, the LUN behaves identically to "disk",
except that generic SCSI commands from the guest are accepted
and passed through to the physical device. Also note that
--
2.1.0
1:44 p.m.
New subject: [libvirt] [PATCH 1/2] docs: Clarification for when allowed to use 'lun' for "volume"
On 06/26/2015 02:15 PM, John Ferlan wrote:
While re-reading what I wrote for commit id '785a8940e', I
realized
I needed to clarify that being able to present as a 'lun', the mode
property for the pool source element needed to be "host" (or empty)
and not "direct".
It was described correctly later in the mode host description, but
this just ensures it's not missed here as well.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
docs/formatdomain.html.in | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
ACK (if you needed it)
1:15 p.m.
New subject: [libvirt] [PATCH 2/2] conf: Validate disk lun using correct types
https://bugzilla.redhat.com/show_bug.cgi?id=1201143
The formatdomain.html.in description for <disk> device 'lun' indicates that
it must be either a type 'block' or type 'network' with protocol
'iscsi';
however, we did not make that check until domain startup.
This caused issues for virt-manager which had an unexpected failure at
run time rather config time.
This patch adds a check in post part disk device checking for the specific
and supported lun types as well as adjusting the test failure to be for
parse config rather than run time.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 +-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2c3b96b..211aa9f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3893,6 +3893,28 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
return -1;
}
}
+
+ /* Validate LUN configuration
+ * NOTE: virStorageTranslateDiskSourcePool is not run yet, so for
+ * disk "volume"'s, the closest we can get at config time is
+ * to ensure mode isn't direct since host/default will allow
+ * lun/block usage. At run time if it's determined the wrong
+ * voltype and pooltype values are set, then failure occurs
+ */
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN &&
+ !(disk->src->type == VIR_STORAGE_TYPE_BLOCK ||
+ (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
+ disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) ||
+ (disk->src->type == VIR_STORAGE_TYPE_VOLUME &&
+ disk->src->srcpool &&
+ disk->src->srcpool->mode !=
+ VIR_STORAGE_SOURCE_POOL_MODE_DIRECT))) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk '%s' improperly configured to be a
"
+ "device='lun'"),
+ disk->dst);
+ return -1;
+ }
}
if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) {
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a212d30..43c2769 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -818,7 +818,7 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-no-boot",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_BOOTINDEX);
- DO_TEST_FAILURE("disk-device-lun-type-invalid",
+ DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",
QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_SCSI);
DO_TEST("disk-usb", NONE);
DO_TEST("disk-usb-device",
--
2.1.0
1:51 p.m.
New subject: [libvirt] [PATCH 2/2] conf: Validate disk lun using correct types
On 06/26/2015 02:15 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1201143
The formatdomain.html.in description for <disk> device 'lun' indicates that
you could just say formatdomain.html...
it must be either a type 'block' or type 'network'
with protocol 'iscsi';
however, we did not make that check until domain startup.
This caused issues for virt-manager which had an unexpected failure at
run time rather config time.
This patch adds a check in post part disk device checking for the specific
and supported lun types as well as adjusting the test failure to be for
parse config rather than run time.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 +-
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2c3b96b..211aa9f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3893,6 +3893,28 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
return -1;
}
}
+
+ /* Validate LUN configuration
+ * NOTE: virStorageTranslateDiskSourcePool is not run yet, so for
+ * disk "volume"'s, the closest we can get at config time
is
+ * to ensure mode isn't direct since host/default will allow
+ * lun/block usage. At run time if it's determined the wrong
+ * voltype and pooltype values are set, then failure occurs
+ */
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN &&
+ !(disk->src->type == VIR_STORAGE_TYPE_BLOCK ||
+ (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
+ disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) ||
+ (disk->src->type == VIR_STORAGE_TYPE_VOLUME &&
+ disk->src->srcpool &&
+ disk->src->srcpool->mode !=
+ VIR_STORAGE_SOURCE_POOL_MODE_DIRECT))) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk '%s' improperly configured to be a
"
+ "device='lun'"),
maybe "improperly configured for ..."? Either way (or any other way).
Under the assumption that you understand the reasoning behind the logic
much better than I do, ACK.
3437
days inactive
3437
days old
4 comments
2 participants
participants (2)
-
John Ferlan -
Laine Stump