[libvirt] [PATCH 0/2] Add configuration time check for invalid lun settings
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
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@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
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@redhat.com> --- docs/formatdomain.html.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK (if you needed it)
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@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
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@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.
participants (2)
-
John Ferlan -
Laine Stump