This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add locale/charset support to mkshortcut


Andy Koppe wrote:
> Attached is a largish patch for adding locale/charset support to
> mkshortcut and fixing a couple of bugs. It also contains quite a bit
> of code cleanup.

Thanks for your efforts, Andy. I really appreciate it.

> ChangeLog:
> 	* src/mkshortcut/mkshortcut.c:
> 	  - Add locale/charset support and remove Win9x-specific code.
> 	  - Use Unicode versions of Windows function.
> 	  - Remove potential buffer overflows due to unguarded uses of strcpy.
> 	  - Factor out error-reporting boilerplate.

OK. Except that this "factoring out" is also part of the "pointless
freeing" below.

> 	  - Remove unnecessary function arguments and pointless freeing.

I disagree with most of these changes. Also, unless you plan to update
the style of all of the OTHER popt-using utilities in cygutils, I don't
think it is wise to change mkshortcut in this way.

Personally, I dislike global static variables. It's also bad practice
with popt, because a single program can have multiple parsing contexts.
(Not that any of the cygutils tools do this; so if you want to globally
modify all cygutils popt-using utilities to make optCon a static var, I
would accept that patch; but let's do it separately -- but, like "free"
discussion below, we need to preserve the call to poptFreeContext()).

I think that, in general, --help output should go to stdout, not stderr.
 Also --usage, --license, and --version output.  However, if an *error*
is handled by printing the usage(), then it should go to stderr.

That wasn't the case in mkshortcut.  So, rather than removing the FILE *
argument -- since, as you noticed, all of the affected functions were
invariably called with stderr -- I'd prefer that the ?,u,v,l cases be
updated to call the appropriate function with stdout, but that existing
error handling cases call usage() with stderr.  (Ick.  It appears that
most of the other popt-using cygutils tools ALSO send everything always
to stderr. Well, this is an easy change.  Let's punt on this, too, and
I'll do it all-at-once)

Finally, what you call "pointless freeing" I call "good coding
practice". Sure, in each case the very next thing that will happen is
exit() or abort(), so the system will reclaim that memory immediately.
So these "leaks" are harmless -- but they ARE leaks, and
valgrind/duma/purify will report them as such.  If you want to
consolidate error handling, which is probably a good idea, then a better
approach is to register each malloc'ed pointer with a cleanup function
[*] and always call that before exit().

[*] or put them all in a static struct or two [**] (sigh), and use the
XFREE() macro on each member (from a simple cleanup function that you
always call before exit; e.g. from within nullcheck() and error(), as
well as the explicit exit() calls).

Or create individual cleanup functions for each one, and register them
with atexit().

Or combine these two approaches and register a single cleanup function
with atexit().

Or clean them up "locally", as the existing code did.

I realize the problem is harder in your version, because many of the
static buf[] variables are now dynamically allocated, but I still think
it's good practice to clean up own messes, even just before calling exit().

[**] I'm thinking one struct for the vars malloc'ed in main(), and
another for those in mkshortcut().

> 	  - Return zero for success from mkshortcut().

Derr...oops.  Good catch.

--
Chuck


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]