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

[John Ferlan]

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 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.
> 

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