9:43 p.m.
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(a)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(a)redhat.com> wrote:
> On Thu, Dec 14, 2017 at 18:14:36 +0800, lichstor(a)gmail.com wrote:
> > From: zhangshengyu <zhangshengyu(a)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.
>