On 04/05/2017 04:16 PM, Martin Kletzander wrote:
> On Wed, Apr 05, 2017 at 03:11:52PM -0400, John Ferlan wrote:
>>
>>
>> On 04/05/2017 04:50 AM, Michal Privoznik wrote:
>>> This function runs an iscsi command and parses its output.
>>> However, due to the nature of things, virISCSIExtractSession()
>>> callback can be called multiple times. In each run it would
>>> allocate new memory and overwrite the variable where we keep
>>> pointer to it and thus leaking old allocations.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/util/viriscsi.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>>> index 504ffbd..9c6fde0 100644
>>> --- a/src/util/viriscsi.c
>>> +++ b/src/util/viriscsi.c
>>> @@ -52,7 +52,8 @@ virISCSIExtractSession(char **const groups,
>>> {
>>> struct virISCSISessionData *data = opaque;
>>>
>>> - if (STREQ(groups[1], data->devpath))
>>> + if (!data->devpath &&
>>> + STREQ(groups[1], data->devpath))
>>> return VIR_STRDUP(data->session, groups[0]);
>>> return 0;
>>> }
>>>
>>
>> I see you fixed your typo "!data->devpath" to "!data->session" before
>
> I wonder how I missed that, maybe because this wasn't the first time I
> saw the patch O:-)
>
>> pushing, but the reality is this is a no-op considering at most we can
>> only match once, but because virCommandRunRegex only bails if the return
>
> It is not no-op. The function is ran on every line of output and, as
> we've noticed on Michal's machine, he got 2 lines with the same
> group[1], so to speak.
>
So what's the output from a "iscsiadm --mode session" on Michal's
machine? And then if there is a duplicate - how did it happen?
For my iSCSI config on Fedora, the /etc/tgt/targets.conf is what is used
to set up those IQN's... After that it's a lot of iscsid/tgtd magic in
order to allow the iscsiadm "search" commands work.
>> from "*func" is "< 0", we just have to continue through the loop. Not
>> that it should, but for the purpose of this callback it could.
>>
>
> well, virCommandRunRegex() ends when func() returns negative value, but
> it ends as an error. And we wouldn't be able to differentiate between
> real error and "func() already found what it needed". We would have to
> change virCommandRunRegex() to work with yet another return value and
> behave based on that.
>
Well logic could be added to checking of the return vs. 0 or 1; where 0
is keep going and 1 is stop processing we found what we wanted. I tend
to try and not think to hard about that regex code - in fact the more I
can forget the better off I am ;-)
John
>> In any case, we're guaranteed that the "groups[1]" will always be
>> unique, so all the code does is scan the output looking for the matching
>> session # and then copy that into data->session. Each session must have
>> a unique string as that the IQN which must be unique.
>>
>
> Then there is an error on Michal's machine then. In any case, it won't
> hurt if we don't depend on external tools behaving as predicted. It's
> the same thing as segfaulting, we shouldn't segfault on any external
> input, no matter how wrong it is.
>
>> As an aside, data->session could change to an int, but that would
>> require some other changes as well.
>>
>> John
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
