From that list, the target can be found. This operation can also fill an

On Mon, Jul 18, 2016 at 07:37:20AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1356436 > > According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are > two ways to "discover" targets in/for the iSCSI environment. Discovery > is the process which allows the initiator to find the targets to which > it has access and at least one address at which each target may be > accessed. > > The method currently implemented in libvirt using the virISCSIScanTargets > API is known as "SendTargets" discovery. This method is more useful when > the target IP Address and TCP port information are available, e.g. in > libvirt terms the "portal". It returns a list of targets for the portal. >> From that list, the target can be found. This operation can also fill an > iSCSI node table into which iSCSI logins may occur. Commit id '56057900' > altered that filling by adding the "--op nonpersistent" since it was > not necessarily desired to perform that for non libvirt related targets. > > The second method is "Static Configuration". This method not only needs > the IP Address and TCP port (e.g. portal), but also the iSCSI target > name. > In libvirt terms this would be the device path field from the iSCSI pool > <source> XML. This patch implements the second methodology using that > required device path as the targetname. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/util/viriscsi.c | 45 > +++++++++++++++++++++++++++++++++++++++++++++ > src/util/viriscsi.h | 6 ++++++ > 3 files changed, 52 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 3580a72..edef70b 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; > virISCSIConnectionLogin; > virISCSIConnectionLogout; > virISCSIGetSession; > +virISCSINodeNew; > virISCSINodeUpdate; > virISCSIRescanLUNs; > virISCSIScanTargets; > diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c > index e705517..8115d09 100644 > --- a/src/util/viriscsi.c > +++ b/src/util/viriscsi.c > @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, > return ret; > } > > +/* > + * virISCSINodeNew: > + * @portal: address for iSCSI target > + * @target: IQN and specific LUN target > + * > + * Usage of nonpersistent discovery in virISCSIScanTargets is useful > primarily > + * only when the target IQN is not known; however, since we have the > target IQN > + * usage of the "--op new" can be done. This avoids problems if "--op > delete" > + * had been used wiping out the static nodes determined by the > scanning of > + * all targets. > + * > + * NB: If already created, subsequent "--op new" commands do not return > + * an error. If it does not return an error, do we need to ignore non-zero status?

AFAIK the rest of the code ignoring exit codes was written before iscsiadm returned them properly: https://github.com/open-iscsi/open-iscsi/commit/2c839a2 Even this libvirt commit from Jan 2010 speaks of 'older versions of iscsiadm': http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6aabcb5b Jan

On Tue, Jul 26, 2016 at 10:25:41PM -0400, John Ferlan wrote: > > > On 07/26/2016 12:24 PM, Ján Tomko wrote: >> On Mon, Jul 18, 2016 at 07:37:20AM -0400, John Ferlan wrote: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1356436 >>> >>> According to RFC 3721 (https://www.ietf.org/rfc/rfc3721.txt), there are >>> two ways to "discover" targets in/for the iSCSI environment. Discovery >>> is the process which allows the initiator to find the targets to which >>> it has access and at least one address at which each target may be >>> accessed. >>> >>> The method currently implemented in libvirt using the >>> virISCSIScanTargets >>> API is known as "SendTargets" discovery. This method is more useful >>> when >>> the target IP Address and TCP port information are available, e.g. in >>> libvirt terms the "portal". It returns a list of targets for the >>> portal. >>>> From that list, the target can be found. This operation can also >>>> fill an >>> iSCSI node table into which iSCSI logins may occur. Commit id >>> '56057900' >>> altered that filling by adding the "--op nonpersistent" since it was >>> not necessarily desired to perform that for non libvirt related >>> targets. >>> >>> The second method is "Static Configuration". This method not only needs >>> the IP Address and TCP port (e.g. portal), but also the iSCSI target >>> name. >>> In libvirt terms this would be the device path field from the iSCSI >>> pool >>> <source> XML. This patch implements the second methodology using that >>> required device path as the targetname. >>> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> src/libvirt_private.syms | 1 + >>> src/util/viriscsi.c | 45 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> src/util/viriscsi.h | 6 ++++++ >>> 3 files changed, 52 insertions(+) >>> >>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>> index 3580a72..edef70b 100644 >>> --- a/src/libvirt_private.syms >>> +++ b/src/libvirt_private.syms >>> @@ -1714,6 +1714,7 @@ iptablesRemoveUdpOutput; >>> virISCSIConnectionLogin; >>> virISCSIConnectionLogout; >>> virISCSIGetSession; >>> +virISCSINodeNew; >>> virISCSINodeUpdate; >>> virISCSIRescanLUNs; >>> virISCSIScanTargets; >>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c >>> index e705517..8115d09 100644 >>> --- a/src/util/viriscsi.c >>> +++ b/src/util/viriscsi.c >>> @@ -444,6 +444,51 @@ virISCSIScanTargets(const char *portal, >>> return ret; >>> } >>> >>> +/* >>> + * virISCSINodeNew: >>> + * @portal: address for iSCSI target >>> + * @target: IQN and specific LUN target >>> + * >>> + * Usage of nonpersistent discovery in virISCSIScanTargets is useful >>> primarily >>> + * only when the target IQN is not known; however, since we have the >>> target IQN >>> + * usage of the "--op new" can be done. This avoids problems if "--op >>> delete" >>> + * had been used wiping out the static nodes determined by the >>> scanning of >>> + * all targets. >>> + * >>> + * NB: If already created, subsequent "--op new" commands do not >>> return >>> + * an error. >> >> If it does not return an error, do we need to ignore non-zero status? > > I thought the point of the comment is that subsequent --op new commands > won't cause an error "if" the node record was generated. IOW: It's ok to > have multiple NodeNew commands and those won't cause an error. If there > was some other failure, then I'd expect to get and report some error. > But for this particular command the code isn't ignoring any error, so > the code isn't ignoring errors... > Perhaps I should have written it below the code, not the above comment: + /* Ignore non-zero status. */ + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed new node mode for target '%s'"), + target); + goto cleanup; + } With a non-NULL second argument, virCommandRun ignores non-zero exit status and expects the caller to deal with it. If we don't need to be compatible with old broken iscsiadm and multiple --op new commands do not return an error, we should error out on non-zero status.

Jan > John >> >> AFAIK the rest of the code ignoring exit codes was written before >> iscsiadm >> returned them properly: >> https://github.com/open-iscsi/open-iscsi/commit/2c839a2 >> >> Even this libvirt commit from Jan 2010 speaks of 'older versions of >> iscsiadm': >> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6aabcb5b >> >> Jan