5:50 a.m.
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(a)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