On 07/27/2016 06:48 AM, Ján Tomko wrote: > 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@xxxxxxxxxx> >>>> --- >>>> 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. > Ahhhh OK - I see your point now. Of course none of this code deals with the exitstatus anyway yet, but I can add something before pushing - John > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list