[libvirt] [PATCH] add support of iSER transport type in qemu with libiscsi
Charles Kelimod
lichstor at gmail.com
Sat Dec 16 03:43:24 UTC 2017
I made clear why it create iscsi+iser, not only that the system have many
place expect iscsi+iser type of strings, like qemuParseDriveURIString.
but actually for qemu command line, iser://example.org... can work while
iscsi+iser will not, how should I do?
Currently for json type of command line in qemu can work for me, if need to
use "iser://" and let disk-drive-network-iser.args to work, need to modify
many place like qemuParseDriveURIString
On Fri, Dec 15, 2017 at 11:47 AM, Charles Kelimod <lichstor at gmail.com>
wrote:
> Thank you for your patience, I regret for my first time submit here that
> brings so may mistakes.
>
> I have done almost changes that you have mentioned. Now I have a question:
> I created drive-file-network-args.xml in test, and there is: -drive
> file=iser://example.org:6000/iqn.1992-01.com.example/1,format=raw
> but the system create -drive file=iscsi+iser://example.org:
> 6000/iqn.1992-01.com.example/1,format=raw
> I doubt why it create iscsi+iser?
>
> On Thu, Dec 14, 2017 at 7:35 PM, Peter Krempa <pkrempa at redhat.com> wrote:
>
>> On Thu, Dec 14, 2017 at 18:14:36 +0800, lichstor at gmail.com wrote:
>> > From: zhangshengyu <zhangshengyu at fusionstack.cn>
>>
>> As pointed out last time, please follow the contributor guidelines. You
>> did not run make syntax-check. Also the contributor guidelines state
>> that you should write a commit message. I asked last time for that too.
>>
>> Please note that I will NOT review any other version until you follow
>> the contributor guidelines at:
>>
>> https://libvirt.org/hacking.html
>>
>> See below for reasons.
>>
>> >
>> > ---
>> > src/conf/domain_conf.c | 13 ++++++++
>> > src/qemu/qemu_block.c | 17 +++++++++-
>> > src/util/virstoragefile.c | 3 +-
>> > src/util/virstoragefile.h | 1 +
>> > tests/qemuxml2argvdata/disk-drive-network-iser.xml | 37
>> ++++++++++++++++++++++
>> > 5 files changed, 69 insertions(+), 2 deletions(-)
>> > create mode 100644 tests/qemuxml2argvdata/disk-drive-network-iser.xml
>> >
>> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> > index 66e21c4bd..bf20cfd0c 100644
>> > --- a/src/conf/domain_conf.c
>> > +++ b/src/conf/domain_conf.c
>> > @@ -7201,6 +7201,9 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr
>> sourcenode,
>> > iscsisrc->src->type = VIR_STORAGE_TYPE_NETWORK;
>> > iscsisrc->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
>> >
>> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> > + _("virDomainHostdevSubsysSCSI
>> iSCSIDefParseXML"));
>> > +
>>
>> Is this leftover debugging? it's certainly aligned wrongly and does not
>> make sense at all.
>>
>> > if (!(iscsisrc->src->path = virXMLPropString(sourcenode, "name")))
>> {
>> > virReportError(VIR_ERR_XML_ERROR, "%s",
>> > _("missing iSCSI hostdev source path name"));
>> > @@ -8416,6 +8419,7 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>> > unsigned int flags)
>> > {
>> > char *protocol = NULL;
>> > + char *transport = NULL;
>> > char *haveTLS = NULL;
>> > char *tlsCfg = NULL;
>> > int tlsCfgVal;
>> > @@ -8427,6 +8431,10 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>> > goto cleanup;
>> > }
>> >
>> > + if (!(transport = virXMLPropString(node, "transport"))) {
>> > + VIR_WARN("missing network source transport type");
>>
>> It will be missing for all other protocols. You can't just do that.
>>
>> > + }
>> > +
>> > if ((src->protocol = virStorageNetProtocolTypeFromString(protocol))
>> <= 0) {
>> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> > _("unknown protocol type '%s'"), protocol);
>> > @@ -8495,6 +8503,9 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>> > if (virDomainStorageNetworkParseHosts(node, &src->hosts,
>> &src->nhosts) < 0)
>> > goto cleanup;
>> >
>> > + if(src->hosts)
>> > + src->hosts->transport = virStorageNetHostTransportType
>> FromString(transport);
>>
>> This is plain wrong. virDomainStorageNetworkParseHosts should parse
>> this.
>>
>> > +
>> > virStorageSourceNetworkAssignDefaultPorts(src);
>> >
>> > ret = 0;
>> > @@ -22326,6 +22337,8 @@ virDomainDiskSourceFormatNetwork(virBufferPtr
>> attrBuf,
>> >
>> > VIR_FREE(path);
>> >
>> > + virBufferEscapeString(attrBuf, " transport='%s'", "iser");
>>
>> What?!?! How is this even supposed to work? Did you even bother running
>> make check?!?! This breaks 9 test suites.
>>
>> > +
>> > if (src->haveTLS != VIR_TRISTATE_BOOL_ABSENT &&
>> > !(flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
>> > src->tlsFromConfig))
>> > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
>> > index 585f0255e..dcd7c6a5e 100644
>> > --- a/src/qemu/qemu_block.c
>> > +++ b/src/qemu/qemu_block.c
>> > @@ -506,6 +506,20 @@ qemuBlockStorageSourceBuildJSO
>> NSocketAddress(virStorageNetHostDefPtr host,
>> > goto cleanup;
>> > break;
>> >
>> > + case VIR_STORAGE_NET_HOST_TRANS_ISER:
>> > + transport = "iser";
>> > + if (virAsprintf(&port, "%u", host->port) < 0)
>> > + goto cleanup;
>> > +
>> > + if (virJSONValueObjectCreate(&server,
>> > + "s:type", transport,
>> > + "s:host", host->name,
>> > + "s:port", port,
>> > + NULL) < 0)
>> > + goto cleanup;
>> > +
>> > +
>> > + break;
>>
>> Missing line break here. Please follow the coding style in the rest of
>> the function.
>>
>> > case VIR_STORAGE_NET_HOST_TRANS_UNIX:
>> > if (virJSONValueObjectCreate(&server,
>> > "s:type", "unix",
>>
>> [...]
>>
>> > diff --git a/tests/qemuxml2argvdata/disk-drive-network-iser.xml
>> b/tests/qemuxml2argvdata/disk-drive-network-iser.xml
>> > new file mode 100644
>> > index 000000000..b3f4f9bfb
>> > --- /dev/null
>> > +++ b/tests/qemuxml2argvdata/disk-drive-network-iser.xml
>>
>> You are missing the output file and change to qemuxml2argvtest.c, thus
>> this is testing nothing. I also think that it will not work for most
>> cases unless you tweak qemuDiskSourceNeedsProps.
>>
>> Also this fails to compile when the gluster driver is enabled:
>>
>> storage/storage_backend_gluster.c: In function
>> 'virStorageFileBackendGlusterInitServer':
>> storage/storage_backend_gluster.c:600:5: error: enumeration value
>> 'VIR_STORAGE_NET_HOST_TRANS_ISER' not handled in switch [-Werror=switch]
>> switch ((virStorageNetHostTransport) host->transport) {
>> ^~~~~~
>>
>> Do not send any other version until you fix the tests. You also need to
>> run make syntax-check and fix any problems it points out.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171216/b32695f6/attachment-0001.htm>
More information about the libvir-list
mailing list