> Date: Thu, 22 Nov 2001 21:27:24 -0700
> From: Richard Gooch <rgooch@xxxxxxxxxxxxxxx>
> Cc: devfs@xxxxxxxxxxx
> References: <200111162309.AAA05838@xxxxxxxxxxxxxxxxxxxxxxxx>
> Content-Length: 2392
>
> Yann Droneaud writes:
> > here is a quite big patch against devfsd v1.3.18.
>
> Yes, too big. It does more than one thing, so it makes it harder for
> me to pick and choose things I like and reject things I don't like.
> Since there's bits I didn't like, the whole patch is rejected.
>
Ok, but i don't think i can split it, since all things are related.
> > This enable user to select which modprobe to use,
> > This also let the user specify arguments to pass to it.
> > The config options could be put in devfsd.conf or on command line.
> > Command line options have a higher priority.
>
> I think this level of flexbility isn't needed. I have yet to see a
> good reason why modprobe isn't in /sbin. Furthermore, adding these
> configuration options bloats the code more. I'm taking a hard line
> with extra code, because I want devfsd to be very small. Think
> embedded systems.
Yes it's not designed for embedded target,
but it could be a build time option (a #define FULL_DEVFSD)
to make it more versatile. It's up to you. If you decided it's really
not interesting, don't include those functionnalities.
I will use them for my private use (quite limited, i could simply hard code
the modprobe and flags i wanted)
>
> I'll make some comments about things I noticed in the patch, so you
> can get an idea of my likes and dislikes.
>
> > + Last updated by Yann Droneaud <ydroneaud@xxxxxxxxxxx> 16-NOV-2001:
> > support for
> > + modprobe configuration (which modprobe use and with what parameters)
>
> Including the email address isn't the style I like: it fouls up the
> formatting.
>
> > -#define MAX_ARGS (6 + 1)
> > +#define MAX_ARGS (6 + 1)
> > +#define MAX_ARGV_ARGS 16 /* number of arguments passed to modprobe */
> > #define MAX_SUBEXPR 10
>
> Gratuitous formatting change. Not something that should be included
> with a patch that makes functional changes.
>
> > - char *argv[MAX_ARGS + 1]; /* argv[0] must always be the programme */
> > + char *argv[MAX_ARGS + 1]; /* argv[0] must always be the program */
>
> Grrr, "programme" is *correct* spelling. This is just annoying. Before
> attempting to correct my spelling, check with the OED (Oxford English
> Dictionary).
Sorry, i didn't know that correct spelling is 'programme', this sound too
much like the french word. This is why i 'corrected' them.
>
> > + if (modprobe.path_config[0] != '\0')
> > + return modprobe.path_config; /* use the one found in config file */
>
> You missed a "the" in the comment.
>
> > + /* can't compute args only one time, must be resetup when devfsd.conf
> > is reread (SIGHUP) */
>
> "resetup" isn't a real word.
Frenchies like me are not rigorous with the language. This is the only
reasons i see ;)
>
> > + /* compute really dynamic parameter (those that don't depend of us
> > directly) */
>
> Typo? Should be "on" instead of "of"?
>
> > - SYSLOG (LOG_ERR, "error forking for programme: %s\t%s\n",
> > + SYSLOG (LOG_ERR, "error forking for program: %s\t%s\n",
>
> More bogus spelling corrections.
>
;)
> Regards,
>
> Richard....
> Permanent: rgooch@xxxxxxxxxxxxx
> Current: rgooch@xxxxxxxxxxxxxxx
>
>
Thank you for your comments, I will try to handle them.
--
Yann Droneaud <ydroneaud@xxxxxxxxxxx> +33 6 88 40 82 43
<ydroneaud@xxxxxxxxxxxxx> <yann.droneaud@xxxxxxxxxxxxxxxxxxxxxx>
1024D/BEA43321 5D91 B5B0 5137 B8FE 6882 FE19 CAA0 6F05 BEA4 3321
http://www.meuh.eu.org http://www-iupmime.univ-lemans.fr
|