Re: [libvirt] [PATCH] qemu: Provide default LUN=0 for iSCSI if not provided

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(a)redhat.com&gt; >> --- >> 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.