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 <mpriv...@redhat.com> >>> --- >>> 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list