[libvirt] [PATCH] qemu: Provide default LUN=0 for iSCSI if not provided
https://bugzilla.redhat.com/show_bug.cgi?id=1477880 If the "/#" is missing from the provided iSCSI path, then we need to provide the default LUN of /0; otherwise, QEMU will fail to parse the URL causing a failure to either create the guest or hotplug attach the storage. Alter the command lines generated appropriately. This change does not effect the XML input files. One 'side effect' of this for the argv2xml is that if a "/0" is found on the command line, then the generated XML would need to add the "/0" to the output XML since it wouldn't be known whether it existed or not at qemu process creation time. Signed-off-by: John Ferlan <jferlan@redhat.com> --- NB: Investigated the history quite a bit between QEMU and libiscsi and it seems as though this probably never worked and of course never "tested" in a real environment. Ironically, in qapi/block-core.json, it is claimed that the LUN would default to 0, but the block/iscsi.c code that calls iscsi_parse_full_url would never (AFAICT) have supported not supplying some LUN on the command line. src/qemu/qemu_command.c | 20 ++++++++++++++++---- .../qemuargv2xml-disk-drive-network-iscsi.args | 2 +- .../qemuargv2xml-disk-drive-network-iscsi.xml | 2 +- .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-lsi-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-virtio-iscsi.args | 2 +- 7 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ed8cb6e..cff84de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -830,10 +830,22 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, src->volume, src->path) < 0) goto cleanup; } else { - if (virAsprintf(&uri->path, "%s%s", - src->path[0] == '/' ? "" : "/", - src->path) < 0) - goto cleanup; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI && + !strchr(src->path, '/')) { + + /* The details of iqn is defined by RFC 3720 and 3721, but + * we just need to make sure there's a lun provided. If not + * provided, then default to zero. If ISCSI LUN is provided + * by /dev/disk/by-path/... , then that path will have the + * specific lun requested. */ + if (virAsprintf(&uri->path, "/%s/0", src->path) < 0) + goto cleanup; + } else { + if (virAsprintf(&uri->path, "%s%s", + src->path[0] == '/' ? "" : "/", + src->path) < 0) + goto cleanup; + } } } diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args index 62cc5c0..ec2e8dc 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args @@ -16,7 +16,7 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --drive file=iscsi://example.org:6000/iqn.1992-01.com.example,format=raw,\ +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,format=raw,\ if=virtio \ -drive file=iscsi://example.org:6000/iqn.1992-01.com.example/1,format=raw,\ if=virtio \ diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml index 23542fa..694412b 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml @@ -16,7 +16,7 @@ <emulator>/usr/bin/qemu-system-i686</emulator> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <source protocol='iscsi' name='iqn.1992-01.com.example'> + <source protocol='iscsi' name='iqn.1992-01.com.example/0'> <host name='example.org' port='6000'/> </source> <target dev='vda' bus='virtio'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args index b25f3a0..0fbfd9a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args @@ -21,7 +21,7 @@ server,nowait \ -boot c \ -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ -usb \ --drive file=iscsi://example.org:3260/iqn.1992-01.com.example,format=raw,\ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,format=raw,\ if=none,id=drive-scsi0-0-0-0 \ -device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args index a1d93af..ed15fda 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args @@ -19,7 +19,7 @@ server,nowait \ -no-acpi \ -boot c \ -usb \ --drive file=iscsi://example.org:6000/iqn.1992-01.com.example,format=raw,\ +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,format=raw,\ if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ id=virtio-disk0 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.args index 43c555a..07ae9f5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi.args @@ -22,7 +22,7 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --drive file=iscsi://example.org:3260/iqn.1992-01.com.example,if=none,\ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,if=none,\ format=raw,id=drive-hostdev0 \ -device scsi-generic,bus=scsi0.0,scsi-id=4,drive=drive-hostdev0,id=hostdev0 \ -drive file=iscsi://example.org:3260/iqn.1992-01.com.example/1,if=none,\ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.args index a78e309..d80c859 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi.args @@ -22,7 +22,7 @@ server,nowait \ -usb \ -drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --drive file=iscsi://example.org:3260/iqn.1992-01.com.example,if=none,\ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,if=none,\ format=raw,id=drive-hostdev0 \ -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ drive=drive-hostdev0,id=hostdev0 \ -- 2.9.5
On Wed, Sep 06, 2017 at 12:05:32 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1477880
If the "/#" is missing from the provided iSCSI path, then we need to provide the default LUN of /0; otherwise, QEMU will fail to parse the URL causing a failure to either create the guest or hotplug attach the storage.
Alter the command lines generated appropriately. This change does not effect the XML input files.
One 'side effect' of this for the argv2xml is that if a "/0" is found on the command line, then the generated XML would need to add the "/0" to the output XML since it wouldn't be known whether it existed or not at qemu process creation time.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- NB: Investigated the history quite a bit between QEMU and libiscsi and it seems as though this probably never worked and of course never "tested" in a real environment. Ironically, in qapi/block-core.json, it is claimed that the LUN would default to 0, but the block/iscsi.c code that calls iscsi_parse_full_url would never (AFAICT) have supported not supplying some LUN on the command line.
src/qemu/qemu_command.c | 20 ++++++++++++++++---- .../qemuargv2xml-disk-drive-network-iscsi.args | 2 +- .../qemuargv2xml-disk-drive-network-iscsi.xml | 2 +- .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-lsi-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-virtio-iscsi.args | 2 +- 7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ed8cb6e..cff84de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -830,10 +830,22 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, src->volume, src->path) < 0) goto cleanup; } else { - if (virAsprintf(&uri->path, "%s%s", - src->path[0] == '/' ? "" : "/", - src->path) < 0) - goto cleanup; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI && + !strchr(src->path, '/')) { + + /* The details of iqn is defined by RFC 3720 and 3721, but + * we just need to make sure there's a lun provided. If not + * provided, then default to zero. If ISCSI LUN is provided + * by /dev/disk/by-path/... , then that path will have the + * specific lun requested. */ + if (virAsprintf(&uri->path, "/%s/0", src->path) < 0) + goto cleanup; + } else { + if (virAsprintf(&uri->path, "%s%s", + src->path[0] == '/' ? "" : "/", + src->path) < 0) + goto cleanup; + } } }
The command line generator is not really the correct place to add defaults. We have the post parse callbacks for this, and the LUN clearly should be marked in the XML
On 09/11/2017 07:26 AM, Peter Krempa wrote:
On Wed, Sep 06, 2017 at 12:05:32 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1477880
If the "/#" is missing from the provided iSCSI path, then we need to provide the default LUN of /0; otherwise, QEMU will fail to parse the URL causing a failure to either create the guest or hotplug attach the storage.
Alter the command lines generated appropriately. This change does not effect the XML input files.
One 'side effect' of this for the argv2xml is that if a "/0" is found on the command line, then the generated XML would need to add the "/0" to the output XML since it wouldn't be known whether it existed or not at qemu process creation time.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- NB: Investigated the history quite a bit between QEMU and libiscsi and it seems as though this probably never worked and of course never "tested" in a real environment. Ironically, in qapi/block-core.json, it is claimed that the LUN would default to 0, but the block/iscsi.c code that calls iscsi_parse_full_url would never (AFAICT) have supported not supplying some LUN on the command line.
src/qemu/qemu_command.c | 20 ++++++++++++++++---- .../qemuargv2xml-disk-drive-network-iscsi.args | 2 +- .../qemuargv2xml-disk-drive-network-iscsi.xml | 2 +- .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-lsi-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-virtio-iscsi.args | 2 +- 7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ed8cb6e..cff84de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -830,10 +830,22 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, src->volume, src->path) < 0) goto cleanup; } else { - if (virAsprintf(&uri->path, "%s%s", - src->path[0] == '/' ? "" : "/", - src->path) < 0) - goto cleanup; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI && + !strchr(src->path, '/')) { + + /* The details of iqn is defined by RFC 3720 and 3721, but + * we just need to make sure there's a lun provided. If not + * provided, then default to zero. If ISCSI LUN is provided + * by /dev/disk/by-path/... , then that path will have the + * specific lun requested. */ + if (virAsprintf(&uri->path, "/%s/0", src->path) < 0) + goto cleanup; + } else { + if (virAsprintf(&uri->path, "%s%s", + src->path[0] == '/' ? "" : "/", + src->path) < 0) + goto cleanup; + } } }
The command line generator is not really the correct place to add defaults. We have the post parse callbacks for this, and the LUN clearly should be marked in the XML
Sure, but doing so would then change the provided XML, right? Is that the desired outcome - to change and save the provided XML? IDC, it just that this point doesn't alter that XML - rather just the command line which seemed a bit safer to me. John Ironically as one digs into QEMU one finds that it claims that the LUN doesn't have to be provided (defaults to 0 in @BlockdevOptionsIscsi seems to imply that to me), but doing so doesn't work. Nor could I find a commit from qemu histories where it ever did work. Even the JSON parsing code in virStorageSourceParseBackingJSONiSCSI seems to be written such that if "lun" isn't there, the XML is built to append a 0 since we don't check status of the read.
On Mon, Sep 11, 2017 at 08:06:04AM -0400, John Ferlan wrote:
On 09/11/2017 07:26 AM, Peter Krempa wrote:
On Wed, Sep 06, 2017 at 12:05:32 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1477880
If the "/#" is missing from the provided iSCSI path, then we need to provide the default LUN of /0; otherwise, QEMU will fail to parse the URL causing a failure to either create the guest or hotplug attach the storage.
Alter the command lines generated appropriately. This change does not effect the XML input files.
One 'side effect' of this for the argv2xml is that if a "/0" is found on the command line, then the generated XML would need to add the "/0" to the output XML since it wouldn't be known whether it existed or not at qemu process creation time.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- NB: Investigated the history quite a bit between QEMU and libiscsi and it seems as though this probably never worked and of course never "tested" in a real environment. Ironically, in qapi/block-core.json, it is claimed that the LUN would default to 0, but the block/iscsi.c code that calls iscsi_parse_full_url would never (AFAICT) have supported not supplying some LUN on the command line.
src/qemu/qemu_command.c | 20 ++++++++++++++++---- .../qemuargv2xml-disk-drive-network-iscsi.args | 2 +- .../qemuargv2xml-disk-drive-network-iscsi.xml | 2 +- .../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +- .../qemuxml2argv-disk-drive-network-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-lsi-iscsi.args | 2 +- .../qemuxml2argv-hostdev-scsi-virtio-iscsi.args | 2 +- 7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ed8cb6e..cff84de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -830,10 +830,22 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, src->volume, src->path) < 0) goto cleanup; } else { - if (virAsprintf(&uri->path, "%s%s", - src->path[0] == '/' ? "" : "/", - src->path) < 0) - goto cleanup; + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI && + !strchr(src->path, '/')) { + + /* The details of iqn is defined by RFC 3720 and 3721, but + * we just need to make sure there's a lun provided. If not + * provided, then default to zero. If ISCSI LUN is provided + * by /dev/disk/by-path/... , then that path will have the + * specific lun requested. */ + if (virAsprintf(&uri->path, "/%s/0", src->path) < 0) + goto cleanup; + } else { + if (virAsprintf(&uri->path, "%s%s", + src->path[0] == '/' ? "" : "/", + src->path) < 0) + goto cleanup; + } } }
The command line generator is not really the correct place to add defaults. We have the post parse callbacks for this, and the LUN clearly should be marked in the XML
Sure, but doing so would then change the provided XML, right? Is that the desired outcome - to change and save the provided XML? IDC, it just that this point doesn't alter that XML - rather just the command line which seemed a bit safer to me.
Everything that we pass to QEMU command line should be also in XML, the main reason is migration. If some default value is not in the XML and we change the default value in our code migration to a libvirt where the default value is different may fail or may change ABI. Pavel
participants (3)
-
John Ferlan -
Pavel Hrdina -
Peter Krempa