[libvirt] [PATCH 1/2] util: Introduce virISCSINodeNew

[Ján Tomko]

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 at redhat.com>
>>> ---
>>> 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