[PATCH] modprobe and multiple options to modules

Discussion:

Yann E. MORIN

2005-09-28 11:05:30 UTC

Could you review and comment this patch that allows modprobe to accept more
than one option to modules?

No, no, no. It is borked when there are single/double quotes in
double/single quotes... :-(

I didn't use any existing option parsing as it looked excesively complicated
for what was needed.

Which I should have done... ;-)

OK, sorry for the fuss... I'll re-work my copy...

Yann E. MORIN.

PS. My mails seem to take a long time to make it to the list. Is this
intended behavior? Hmpf...
YEM.

--
Yann E. MORIN
Roaming in the world...

Yann E. MORIN

2005-09-28 18:57:01 UTC

Permalink

Hi all!

Could you review and comment this patch that allows modprobe to accept more
than one option to modules?

I didn't use any existing option parsing as it looked excesively complicated
for what was needed.

On an Intel P4, it adds 236 bytes:
text data bss dec hex filename
3728 20 28 3776 ec0 modprobe.o-orig
3964 20 28 4012 fac modprobe.o-new

On an ARM Xscale, it adds 296 bytes:
text data bss dec hex filename
4676 20 28 4724 1274 modprobe.o-orig
4972 20 28 5020 139c modprobe.o-new

That looks like a lot... :-(

Note: In modprobe.c, there were spaces in place of tabs at two places. I also
replaced them with tabs.

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe_options.patch
Type: text/x-patch
Size: 3115 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20050928/089271a2/busybox-1.01-modprobe_options.bin

Rob Landley

2005-09-28 19:37:48 UTC

Permalink

Post by Yann E. MORIN
Hi all!
Could you review and comment this patch that allows modprobe to accept more
than one option to modules?

...

Post by Yann E. MORIN
That looks like a lot... :-(

Yeah, it is a lot.

This code really should be shared with the shell and getopt(1) code.
Unfortunately, _none_ of these share with each other (and I suspect the
various shells don't either).

I've been meaning to take a stab at writing some generic code to parse a
string into a command line. There's lots of places we could use this,
actually.

The code you've done isn't generic (nothing else is going to use mod_list_t),
but the general concept is. It seems like what you want is the ability to
call some parse_command_string() function and have it append the split-up
results to an argv[] you pass. With details like error handling, bounds
checking on the argv[] array (or possibly dynamically allocating the sucker,
the MODPROBE_OPTIONS_NUMBER is a bit clumsy)...

This is an interesting problem to me and it's adjacent to other work I'm
doing, but I'm not going to have _any_ time to work on it before monday.
Tell you what: can you rename the parse function to parse_command_string(),
move the do() loop outside so it doesn't have to know about the
module-specific structure? That way we can easily move this function to a
separate file later to do more work on it and re-use it elsewhere, but for
right now we can just check in a fix to modprobe without blocking the fix on
the later work.

Sound good?

Rob

Yann E. MORIN

2005-09-29 06:00:34 UTC

Permalink

Hi list!
Rob,

Post by Rob Landley
The code you've done isn't generic (nothing else is going to use mod_list_t),
but the general concept is. It seems like what you want is the ability to
call some parse_command_string() function and have it append the split-up
results to an argv[] you pass. With details like error handling, bounds
checking on the argv[] array

OK, see below.

Post by Rob Landley
(or possibly dynamically allocating the sucker,
the MODPROBE_OPTIONS_NUMBER is a bit clumsy)...

Yeah, that was a quick fix, just for validating the parsing code.

Post by Rob Landley
Tell you what: can you rename the parse function to parse_command_string(),
move the do() loop outside so it doesn't have to know about the
module-specific structure? That way we can easily move this function to a
separate file later to do more work on it and re-use it elsewhere,

OK, I'm re-factoring it.

Post by Rob Landley
but for
right now we can just check in a fix to modprobe without blocking the fix on
the later work.

Yes, but the patch I sent is, IMHO, twisted because it doesn't allow quotes
nesting. Plus it keeps the quotes around arguments, which it shouldn't. Plus
it doesn't do escaping. Plus... Bahhh...

Anyway, thanks for the review, I'll take your word into account. I'm putting
together all this in some new code that will hopefully be usefull to you.

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...

Rob Landley

2005-09-29 15:37:15 UTC

Permalink

Post by Yann E. MORIN
Anyway, thanks for the review, I'll take your word into account. I'm
putting together all this in some new code that will hopefully be usefull
to you.

Sounds like you've got a good handle on things. I look forward to seeing the
result...

Rob

Yann E. MORIN

2005-09-30 04:30:47 UTC

Permalink

Hi!

Post by Rob Landley
Sounds like you've got a good handle on things. I look forward to seeing the
result...

Hmm. Looks like not to me. As of now, I've broken the string parsing into
finding the begining of an argument (the first char in the string that is not
a space), and the end of the argument (the next first char that is not escaped
and not between quotes). Single- and double-quotes evaluation (not yet fully
compliant to traditional sh I guess...) Plus some escaping evaluation ('\"',
'\'', '\ ', plus all the \a\b\t\n\v\f\r\0.

So far the original command string was modified by the function, and I don't
find it very interesting because when we want variables expansion, then we
would need to expand/shrink the string at will. So I started xstrdup'ing the
original string to get a working copy of the current argument (with trailing
garbage being the remaining args).

It is the responsibility of the caller to iterate over the arguments, and
free the xstrdup'ed arguments. And I conditionned the memory freeing to
CONFIG_FEATURE_CLEAN_UP being defined (or we leak memory when looping for
dependencies).

But for now it dumps core... :-(

static char* parse_command_string( char* src, char **dst );
/* src: pointer to string containing options
* dst: pointer to where to store the parsed option
* return value: the pointer to the first char after the parsed option,
* NULL if there was no option parsed (only trailing spaces).
* Note that memory is allocated with bb_xstrdup when a new argument
* was parsed. Don't forget to free it!
*/

Tonight for a status update (or earlier when I have something working!).

Regards,
Yann.

--
Yann E. MORIN
Roaming in the world...

Tito

2005-09-30 10:08:44 UTC

Permalink

Post by Yann E. MORIN
Hi!

Post by Rob Landley
Sounds like you've got a good handle on things. I look forward to seeing the
result...

Hmm. Looks like not to me. As of now, I've broken the string parsing into
finding the begining of an argument (the first char in the string that is not
a space), and the end of the argument (the next first char that is not escaped
and not between quotes). Single- and double-quotes evaluation (not yet fully
compliant to traditional sh I guess...) Plus some escaping evaluation ('\"',
'\'', '\ ', plus all the \a\b\t\n\v\f\r\0.
So far the original command string was modified by the function, and I don't
find it very interesting because when we want variables expansion, then we
would need to expand/shrink the string at will. So I started xstrdup'ing the
original string to get a working copy of the current argument (with trailing
garbage being the remaining args).
It is the responsibility of the caller to iterate over the arguments, and
free the xstrdup'ed arguments. And I conditionned the memory freeing to
CONFIG_FEATURE_CLEAN_UP being defined (or we leak memory when looping for
dependencies).
But for now it dumps core... :-(
static char* parse_command_string( char* src, char **dst );
/* src: pointer to string containing options
* dst: pointer to where to store the parsed option
* return value: the pointer to the first char after the parsed option,
* NULL if there was no option parsed (only trailing spaces).
* Note that memory is allocated with bb_xstrdup when a new argument
* was parsed. Don't forget to free it!
*/
Tonight for a status update (or earlier when I have something working!).
Regards,
Yann.
--
Yann E. MORIN

Hi,
would be nice if you attach also the code,
so we could take a look at it.

Ciao,
Tito

Yann E. MORIN

2005-09-30 10:45:27 UTC

Permalink

Hi list!
Tito, Rob,

Post by Tito
would be nice if you attach also the code,
so we could take a look at it.

Sure, but I wanted to submit something that would be usefull.
And I'm not quite sure I succeded at that... Anyway, here is what I have
so far. It's against 1.01, and it is *HUGE* (to me)!

It works (seems to) when modprobe is called being verbose. When it is
quiet, it dumps core when (line 525 when applied to 1.01):
argv = (char**) xrealloc( argv, argc * sizeof( char* ) );

Comments are most welcome! ;-)

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe.patch.bz2
Type: application/x-bzip
Size: 2334 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20050930/69ba41ea/busybox-1.01-modprobe.patch.bin

Yann E. MORIN

2005-09-30 12:45:06 UTC

Permalink

Hi list!
Tito, Rob,

Post by Tito
would be nice if you attach also the code,
so we could take a look at it.

Sure, but I wanted to submit something that would be usefull.
And I'm not quite sure I succeded at that... Anyway, here is what I have
so far. It's against 1.01, and it is *HUGE* (to me)!

It works (seems to) when modprobe is called being verbose. When it is
quiet, it dumps core when (line 525 when applied to 1.01):
argv = (char**) xrealloc( argv, argc * sizeof( char* ) );

Comments are most welcome! ;-)

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe.patch.bz2
Type: application/x-bzip
Size: 2334 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20050930/69ba41ea/attachment.bin

Tito

2005-09-30 12:07:51 UTC

Permalink

Post by Yann E. MORIN
Hi!

Post by Rob Landley
Sounds like you've got a good handle on things. I look forward to seeing the
result...

Hmm. Looks like not to me. As of now, I've broken the string parsing into
finding the begining of an argument (the first char in the string that is not
a space), and the end of the argument (the next first char that is not escaped
and not between quotes). Single- and double-quotes evaluation (not yet fully
compliant to traditional sh I guess...) Plus some escaping evaluation ('\"',
'\'', '\ ', plus all the \a\b\t\n\v\f\r\0.
So far the original command string was modified by the function, and I don't
find it very interesting because when we want variables expansion, then we
would need to expand/shrink the string at will. So I started xstrdup'ing the
original string to get a working copy of the current argument (with trailing
garbage being the remaining args).
It is the responsibility of the caller to iterate over the arguments, and
free the xstrdup'ed arguments. And I conditionned the memory freeing to
CONFIG_FEATURE_CLEAN_UP being defined (or we leak memory when looping for
dependencies).
But for now it dumps core... :-(
static char* parse_command_string( char* src, char **dst );
/* src: pointer to string containing options
* dst: pointer to where to store the parsed option
* return value: the pointer to the first char after the parsed option,
* NULL if there was no option parsed (only trailing spaces).
* Note that memory is allocated with bb_xstrdup when a new argument
* was parsed. Don't forget to free it!
*/
Tonight for a status update (or earlier when I have something working!).
Regards,
Yann.
--
Yann E. MORIN

Hi,
would be nice if you attach also the code,
so we could take a look at it.

Ciao,
Tito

Yann E. MORIN

2005-09-30 06:30:27 UTC

Permalink

Hi!

Post by Rob Landley
Sounds like you've got a good handle on things. I look forward to seeing the
result...

Rob Landley

2005-09-29 17:36:41 UTC

Permalink

Post by Yann E. MORIN
Anyway, thanks for the review, I'll take your word into account. I'm
putting together all this in some new code that will hopefully be usefull
to you.

Sounds like you've got a good handle on things. I look forward to seeing the
result...

Rob

Yann E. MORIN

2005-09-29 06:53:40 UTC

Permalink

Hi list!
Rob,

OK, see below.

Post by Rob Landley
(or possibly dynamically allocating the sucker,
the MODPROBE_OPTIONS_NUMBER is a bit clumsy)...

Yeah, that was a quick fix, just for validating the parsing code.

OK, I'm re-factoring it.

Post by Rob Landley
but for
right now we can just check in a fix to modprobe without blocking the fix on
the later work.

Yann E. MORIN

2005-09-28 13:05:01 UTC

Permalink

Could you review and comment this patch that allows modprobe to accept more
than one option to modules?

No, no, no. It is borked when there are single/double quotes in
double/single quotes... :-(

I didn't use any existing option parsing as it looked excesively complicated
for what was needed.

Rob Landley

2005-09-28 21:37:24 UTC

Permalink

Post by Yann E. MORIN
Hi all!
Could you review and comment this patch that allows modprobe to accept more
than one option to modules?

...

Post by Yann E. MORIN
That looks like a lot... :-(

Yann E. MORIN

2005-10-03 10:51:39 UTC

Permalink

Hello!

To Rob's demand, here is the patch I did against 1.01 for modprobe to support
multiple options to modules.

This patch adds a HUGE amount of code, but hopefully should allow to
have a common ground to pasre arguments with a single function, whether
it is called from a shell interpreter, or from any other application needing
argument parsing. Rob, was it what you wanted?

Silly me, in the previous patch (the one that dumped core), I made a error
of one in the size of the array containing argv. Bahh. That was the end of
last week, I guess I was tired...

Anyway, any comments (positive or negative!) are most welcome!

For Intel x86 (P4), it adds 562 bytes:
text data bss dec hex filename
3728 20 28 3776 ec0 modprobe.o-orig
4290 20 28 4338 10f2 modprobe.o

For ARM Xscale (IXP425), it adds 684:
text data bss dec hex filename
4676 20 28 4724 1274 modprobe.o-orig
5360 20 28 5408 1520 modprobe.o

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...

Yann E. MORIN

2005-10-03 13:12:33 UTC

Permalink

Hello!

(Re-post, as my previous message doesn't seem to have made it to the ML).

To Rob's demand, here is the patch I did against 1.01 for modprobe to support
multiple options to modules.

This patch adds a HUGE amount of code, but hopefully should allow to
have a common ground to parse arguments with a single function, whether
it is called from a shell interpreter, or from any other application needing
argument parsing. Rob, was it what you wanted?

Silly me, in the previous patch (the one that dumped core), I made a error
of one in the size of the array containing argv. Bahh. That was the end of
last week, I guess I was tired...

Anyway, any comments (positive or negative!) are most welcome!

For Intel x86 (P4), it adds 562 bytes:
text data bss dec hex filename
3728 20 28 3776 ec0 modprobe.o-orig
4290 20 28 4338 10f2 modprobe.o

For ARM Xscale (IXP425), it adds 684:
text data bss dec hex filename
4676 20 28 4724 1274 modprobe.o-orig
5360 20 28 5408 1520 modprobe.o

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe.patch.bz2
Type: application/x-bzip
Size: 2630 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20051003/a575c4bf/busybox-1.01-modprobe.patch.bin

Andreas Banze

2005-10-03 13:25:36 UTC

Permalink

Post by Yann E. MORIN
(Re-post, as my previous message doesn't seem to have made it to the ML).

it did...

MfG
Andreas Banze

Rob Landley

2005-10-04 15:23:58 UTC

Permalink

Post by Yann E. MORIN
This patch adds a HUGE amount of code, but hopefully should allow to
have a common ground to parse arguments with a single function, whether
it is called from a shell interpreter, or from any other application
needing argument parsing. Rob, was it what you wanted?

You're right the patch is biggish, I need a little more time than I've got
right now to go over it properly. (Hopefully later tonight.) A quick glance
looks vaguely sane, although I've seen a couple potential places to tighten
it up already. I think we can use this...

Query: right at the start you're checking NULL. Are we ever going to call it
with a NULL? (This could be a "don't do that then" condition...)

Post by Yann E. MORIN
text data bss dec hex filename
3728 20 28 3776 ec0 modprobe.o-orig
4290 20 28 4338 10f2 modprobe.o
text data bss dec hex filename
4676 20 28 4724 1274 modprobe.o-orig
5360 20 28 5408 1520 modprobe.o

That's reasonable for this functionality, but big enough that modprobe might
need a config option to support this...

Post by Yann E. MORIN
Regards,
Yann E. MORIN.

Rob

Yann E. MORIN

2005-10-04 19:16:24 UTC

Permalink

Hi all!
Rob,

Post by Rob Landley
You're right the patch is biggish, I need a little more time than I've got
right now to go over it properly. (Hopefully later tonight.) A quick glance
looks vaguely sane, although I've seen a couple potential places to tighten
it up already. I think we can use this...

Good, if a few or more bytes can be reclaimed!

Post by Rob Landley
Query: right at the start you're checking NULL. Are we ever going to call it
with a NULL? (This could be a "don't do that then" condition...)

Well, I use that in mod_process below, in the while loop. Agreed we could save
the bytes for the check, but my habits made me do the check... :-) In that
case, I should handle the loop another way, but I thought it would be more
compact that way.

Post by Rob Landley
That's reasonable for this functionality, but big enough that modprobe might
need a config option to support this...

Done. It is CONFIG_MODPROBE_MULTIPLE_OPTIONS, depending on CONFIG_MODPROBE.
The resulting code is uggly to me. Is there a better way to do that? Any way,
if CONFIG_MODPROBE_MULTIPLE_OPTIONS is not selected, the previous code is
retained, which means only one option can be passed to a module.

A few quirks mended as well.

This patch is still against 1.01, and supersedes my previous patches on the
matter.

Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software Designer | \ / CAMPAIGN | ^ |
| --==< ?_? >==-- ?---.----------------: X AGAINST | /e\ There is no |
| web: ymorin.free.fr | ***@home 3808 | / \ HTML MAIL | """ conspiracy. |
?---------------------?----------------?------------------?--------------------?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe.patch.bz2
Type: application/x-bzip2
Size: 3016 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20051004/83a7f2dc/busybox-1.01-modprobe.patch.bin

Atsushi Nemoto

2005-10-17 06:35:11 UTC

Permalink

yann> Done. It is CONFIG_MODPROBE_MULTIPLE_OPTIONS, depending on
yann> CONFIG_MODPROBE. The resulting code is uggly to me. Is there a
yann> better way to do that? Any way, if
yann> CONFIG_MODPROBE_MULTIPLE_OPTIONS is not selected, the previous
yann> code is retained, which means only one option can be passed to a
yann> module.

yann> A few quirks mended as well.

yann> This patch is still against 1.01, and supersedes my previous
yann> patches on the matter.

I noticed this problem ("modprobe counld not pass multiple arguments")
on busybox 1.01 today and found your patch on ML archive. It works
for me, thank you. If I had not found your mail, I was to start
debugging my driver ...

IMHO, this is really a BUG and CONFIG_MODPROBE_MULTIPLE_OPTIONS should
be enabled by default (or enabled always). I think modprobe should
warn for multiple options if CONFIG_MODPROBE_MULTIPLE_OPTIONS
disabled.

---
Atsushi Nemoto

Atsushi Nemoto

2005-10-17 08:14:23 UTC

Permalink

Yann E. MORIN

2005-10-04 21:17:52 UTC

Permalink

Hi all!
Rob,

Good, if a few or more bytes can be reclaimed!

Post by Rob Landley
Query: right at the start you're checking NULL. Are we ever going to call it
with a NULL? (This could be a "don't do that then" condition...)

Post by Rob Landley
That's reasonable for this functionality, but big enough that modprobe might
need a config option to support this...

Done. It is CONFIG_MODPROBE_MULTIPLE_OPTIONS, depending on CONFIG_MODPROBE.
The resulting code is uggly to me. Is there a better way to do that? Any way,
if CONFIG_MODPROBE_MULTIPLE_OPTIONS is not selected, the previous code is
retained, which means only one option can be passed to a module.

A few quirks mended as well.

This patch is still against 1.01, and supersedes my previous patches on the
matter.

Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software Designer | \ / CAMPAIGN | ^ |
| --==< ?_? >==-- ?---.----------------: X AGAINST | /e\ There is no |
| web: ymorin.free.fr | SETI at home 3808 | / \ HTML MAIL | """ conspiracy. |
?---------------------?----------------?------------------?--------------------?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe.patch.bz2
Type: application/x-bzip2
Size: 3016 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20051004/83a7f2dc/attachment.bin

Andreas Banze

2005-10-03 15:25:19 UTC

Permalink

Post by Yann E. MORIN
(Re-post, as my previous message doesn't seem to have made it to the ML).

it did...

MfG
Andreas Banze

Rob Landley

2005-10-04 05:25:16 UTC

Permalink

That's reasonable for this functionality, but big enough that modprobe might
need a config option to support this...

Post by Yann E. MORIN
Regards,
Yann E. MORIN.

Rob

Yann E. MORIN

2005-10-03 15:11:43 UTC

Permalink

Hello!

(Re-post, as my previous message doesn't seem to have made it to the ML).

To Rob's demand, here is the patch I did against 1.01 for modprobe to support
multiple options to modules.

This patch adds a HUGE amount of code, but hopefully should allow to
have a common ground to parse arguments with a single function, whether
it is called from a shell interpreter, or from any other application needing
argument parsing. Rob, was it what you wanted?

Silly me, in the previous patch (the one that dumped core), I made a error
of one in the size of the array containing argv. Bahh. That was the end of
last week, I guess I was tired...

Anyway, any comments (positive or negative!) are most welcome!

For Intel x86 (P4), it adds 562 bytes:
text data bss dec hex filename
3728 20 28 3776 ec0 modprobe.o-orig
4290 20 28 4338 10f2 modprobe.o

For ARM Xscale (IXP425), it adds 684:
text data bss dec hex filename
4676 20 28 4724 1274 modprobe.o-orig
5360 20 28 5408 1520 modprobe.o

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe.patch.bz2
Type: application/x-bzip
Size: 2630 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20051003/a575c4bf/attachment.bin

Yann E. MORIN

2005-10-03 12:50:19 UTC

Permalink

Yann E. MORIN

2005-09-28 11:45:22 UTC

Permalink

Hi all!

Could you review and comment this patch that allows modprobe to accept more
than one option to modules?

I didn't use any existing option parsing as it looked excesively complicated
for what was needed.

On an Intel P4, it adds 236 bytes:
text data bss dec hex filename
3728 20 28 3776 ec0 modprobe.o-orig
3964 20 28 4012 fac modprobe.o-new

On an ARM Xscale, it adds 296 bytes:
text data bss dec hex filename
4676 20 28 4724 1274 modprobe.o-orig
4972 20 28 5020 139c modprobe.o-new

That looks like a lot... :-(

Note: In modprobe.c, there were spaces in place of tabs at two places. I also
replaced them with tabs.

Regards,
Yann E. MORIN.

--
Yann E. MORIN
Roaming in the world...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.01-modprobe_options.patch
Type: text/x-patch
Size: 3115 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20050928/089271a2/attachment.bin