On Jun 7, 2010, at 4:16 PM, Danny Sung wrote:
>
>
> On 6/7/2010 11:38 AM, Jeff Johnson wrote:
>>
>> On Jun 7, 2010, at 2:00 PM, Danny Sung wrote:
>>
>>> My personal choice in things I write is to expect the caller to strdup().
>>> But I understand the reentrancy issue here. (though why you'd be using
>>> popt in a thread is beyond me at this time.. and it does have a poptContext
>>> handle anyway).
>>>
>>
>> popt does not use threads, applications do. so its applications, not popt,
>> that force -lpopt into multi-threaded environments. All that POPT can
>> do is to try and work transparently in both environments.
>
> Yeah, I meant as an app developer... I can't see why you'd want to use popt
> outside of a single thread. That's the only context where reentrancy would
> be an issue while using the popt library, no?
>
Ah ...
There's lots of usage cases for popt arg processing. E.g. RPM
has multiple embeddings and is just starting to explore the mysteries
of multi-threaded development (its taken most of the last year to
get posix mutexes stabilized and attached transparently to ~40 RPM objects)
POPT is used for all option processing and I want "thread safe" engineering.
>
>>> If this is going to continue to be the behavior, I'd suggest a
>>> poptFreeOptArg() call just to highlight the necessity (and deal with type
>>> and NULL pointer checks).
>>>
>>
>> Why add a new method for what is already a well known operation called
>> free(3)?
>>
>> I mean I can add poptFreeArg in 1 #define to popt.h, but why bother?
>
> Because it's not just "free()" It's:
> if(optarg) free((void *)optarg);
>
(aside)
Hehe. FWIW, I deal with "free" issues (and the pesky GCC const warnings)
using a static inline _free() routine. I can will expose my _free() (its already
used pretty heavily internally to POPT) as poptFree() if you wish. So far I've
just
tried not to inflict my C coding fetishism on anyone else. It is *in fact*
just laziness on the part of the developer to test for NULL and add the
silly cast. Developer laziness is not a POPT problem per se.
> Not a huge deal, but a bit of an inconvenience. Plus like I said the added
> function just higlights the need to free the return from the Get call. I
> actually have a str_delete() call in my standard library that encapsulates
> the if. But I still have to call it like: str_delete((char *)optarg) just to
> break the const to satisfy the compiler warnings.
>
>
>> AFAIK, all POPT args are either returned by value (like POPT_ARG_INT) or
>> through malloc'd memory _ALWAYS_. The rules on table callbacks are different
>> than the rules for the no-brainer, in-line loop based getopt(3) like
>> processing.
>
> Yeah, it's fine. As I said it was just a little unexpected when I first
> started using popt. I didn't realize it until I ran a memory checker. And
> from the 'const' type in the Get call I had thought it was a bug in popt.
>
> Looking over the man page right now (popt 1.16), I don't really see where it
> talks about freeing this memory either. It's compounded by the fact that
> there is a poptFreeContext and poptDupArg() calls. Those both imply to me
> that popt is doing memory management for me.
>
> There's also a poptPeekArg() call. Is the app developer expected to free
> that one as well? If so, then the example seems to lead to a memory leak as
> well:
>
> portname = poptGetArg(optCon);
> if((portname == NULL) || !(poptPeekArg(optCon) == NULL))
> usage(optCon, 1, "Specify a single port", ".e.g., /dev/cua0");
>
My guess (not looked, just from memory) is that poptPeekArg _SHOULD_ return
malloc'd arguments.
The underlying issue is that POPT has a ar filtering construct based on the
gawd awful bash syntax that looks like (here's an example from
/usr/lib/rpm/rpmpopt-X.Y)
# [--dbpath DIRECTORY" "use database in DIRECTORY"
rpm alias --dbpath --define '_dbpath !#:+'
The --dbpath option is quite important for RPM and the ^%$^#^$^%#^%#$^% "!#:+"
syntax was quite useful in ripping out _LOTS_ of tedious code in RPM setting
what is essentially just a file path string argument to an option.
The relation to "memory leaks" is that POPT had to start actively parsing
argv[0] arrays in order to pull the next argument, and that turned out
to be easier to do if arguments were malloc'd before return to caller.
So poptPeekArg() _SHOULD_ return malloc'd memory (but may not do so atm
because I know that nothing but RPM has ever really used poptPeekArg() and
I've been fighting to simplify RPM arg handling for the last 10 years, mostly
successfully).
> Neither the return from poptGetArg() or poptPeekArg() are being freed.
>
poptGetOptArg() is definitely malloc'd, that is the valgrind trace I posted.
I deal with the issue like this (again from RPM code in lib/poptALL.c with
spling pugliness left in cut-n-paste):
/* Process all options, whine if unknown. */
while ((rc = poptGetNextOpt(optCon)) > 0) {
const char * optArg = poptGetOptArg(optCon);
/*...@-dependenttrans -observertr...@*/ /* Avoid popt memory leaks. */
optArg = _free(optArg);
/*...@=dependenttrans =observertrans @*/
switch (rc) {
default:
/*...@-nullpass@*/
fprintf(stderr, _("%s: option table misconfigured (%d)\n"),
__progname, rc);
/*...@=nullpass@*/
exit(EXIT_FAILURE);
/*...@notreached@*/ /*...@switchbreak@*/ break;
}
}
The trace was from rpmio/poptIO.c (where I have never bothered
to "fix" the minor annoyance of a POPT memory leak).
> If this is the behavior you want, I'm fine with it. But I think the man page
> needs updating. (Apologies if it's already been updated, but I don't see it
> on my systems).
>
Well technically, POPT CLI argument processing is no memory "leak" since
it happens only once, and the memory involved in argument strings is usually
in 10's of bytes, not otherwise.
But I can/will add anal retentive strict "make check" test cases
in POPT to fix these issues if necessary, and comply with some
"standard" API/ABI.
Note that popt-1.16 adds api-sanity-autotest.pl "shallow testing" from ISPRAS,
the same group that is doing the heavy duty implementation of LSB certification
testing.
Details are in popt/auto/* if interested. Nice Stuff (imho).
73 de Jeff
> If you'd like, I'll be happy to contribute these changes to the man page. It
> just may take me a weeks before I have any free time.
> ______________________________________________________________________
> POPT Library http://rpm5.org
> Developer Communication List [email protected]
______________________________________________________________________
POPT Library http://rpm5.org
Developer Communication List [email protected]
