[Tcd-hackers] your TCD patch

[Robert Millan]

Hi again,

I will look at your patch, but please avoid sending patches like that, they
are likely to be rejected or ignored in many free software projects.

The problem is your patch fixes a bunch of stuff without splitting each
change. As a maintainer, I need to decide which changes we want applied and
which we don't, and it's not easy to find out which hunk belongs to each
change.

Next time, please submit a separate patch for each of your fixes or
improvements, and an explanation for each of them.

Also mind differing it against CVS, there's no point in appliing your patch
in the 2.1.0 released version.

Having a look at the patch:

> # - capturing signals conflicts a bit with gdb and is not quite needed

I agree, but you should ask first for such kind of stuff.

> # - using PATH_MAX length
> 
> +#ifndef PATH_MAX
> +#  define PATH_MAX 4096 /* linux/limits.h */
> +#endif

This is really *wrong* code. It is wrong because:

 - When PATH_MAX is undefined it means your system has no limit on the length
   of a path, and you must _never_ hardcode a define on PATH_MAX for such
   systems. (the GNU system, aka GNU/Hurd doesn't define PATH_MAX, for example)
 - If you wanted to obtain the PATH_MAX macro (or any other macro) for systems
   that provide them, you should _never_ include a linux/ header. All headers
   in <linux/> directory only exist for providing a interface to the C library
   and are not meant to be included by userspace programs. Instead, include
   <sys/limits.h>, which will define limit macros if and _only_ if the system
   has that limit.

Instead, use dynamic buffers when PATH_MAX is undefined. I believe TCD was
correct on this and your patch breaks it.

> # - added the default cdrom device path for Linux w/devfs
> #   (where is the switch in configure?)

We're migrating to SDL for cdrom access to become kernel-independant. SDL
already knows how to find which the first cdrom is, so this change is not
needed.

> # - "Please mail any info to address@hidden" ... should really be changed now
> #   since he is not available, anyway it is not displayed because of
> #   signal handler removal

IIRC this was fixed on CVS, not sure.

Asides from all my comments, the rest of the patch looks very nice. Please
do the following:

 - Split out the parts of the patch I rejected.
 - Update it for current CVS.

When you send a new patch I'll apply it to CVS. And thanks for your
contribution.

-- 
Robert Millan

"[..] but the delight and pride of Aule is in the deed of making, and in the
thing made, and neither in possession nor in his own mastery; wherefore he
gives and hoards not, and is free from care, passing ever on to some new work."

 -- J.R.R.T, Ainulindale (Silmarillion)