[Top] [All Lists]

Re: [PATCH] configuration for modprobe

To: Yann Droneaud <ydroneaud@xxxxxxxxxxx>
Subject: Re: [PATCH] configuration for modprobe
From: Richard Gooch <rgooch@xxxxxxxxxxxxxxx>
Date: Thu, 22 Nov 2001 21:27:24 -0700
Cc: devfs@xxxxxxxxxxx
In-reply-to: <200111162309.AAA05838@xxxxxxxxxxxxxxxxxxxxxxxx>
References: <200111162309.AAA05838@xxxxxxxxxxxxxxxxxxxxxxxx>
Sender: owner-devfs@xxxxxxxxxxx
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.

> 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.

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

> -#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

> +  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.

> +    /* 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.


Permanent: rgooch@xxxxxxxxxxxxx
Current:   rgooch@xxxxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>