Re: [Toybox] [groups] : ! More than one in group is error
On 12/27/2012 10:32:51 PM, Ashwini Sharma wrote: Hi Rob, With your fix, it doesn't segfault now. But does it matter, to give the proper option name in error message. If yes, then the fix fails in that. e.g. when running *./toybox touch -d 12 -r f2 f1* it throws the error message as *touch: No 'r' with 't'*. I feel the error message is not explanatory here. it should give the proper option names in here. my patch had this fix. I hadn't noticed, because you didn't attach a patch. You gave a text description a change, which makes it harder for me to see what changed. Your description quoted some source, but it was badly whitespace damaged and had asterisks around things I thought were out of place pointer dereferencing (I see now it was an attempt to highlight the changed bits, but I didn't get that at the time). The email separated the two versions so much I couldn't keep them on the screen at the same time in my mail reader window, which made comparing what changed difficult. The first time I read it, the first thing I noticed was you added a null pointer check to mask an error that should never happen (if we fall off the end of the list, we set up the data structure wrong), at which point I stopped reading, especially since your problem description did not include a test case I could reproduce. Long story short: Patches are good. Even if I don't wind up using them as-is, they communicate code changes clearly. Now you've given me a test I can reproduce, which is also good. I agree that's the wrong error message, so let me take another look at what your fix did... Ok, first thing you did was switch back off the bits that this option enabled, including any [+abc] grouping. And by doing that you might indeed create a case where the condition causing the conflict no longer exists, so the loop finds no conflict and falls off the end. But in that case it still won't report it correctly. Hmmm... I'm starting to think that [+abc] groupings are a bad design, both because A) it's just as easy to test for FLAG_a|FLAG_b|FLAG_c in the command (which doesn't result in bigger code because the | gets resolved at compile time according to c99), and B) it makes figuring out _which_ option had a problem (after the fact) hard, because we didn't record which options we saw, just which bits got enabled as a result. I just committed another attempt at a fix. Does this look reasonable? (Since a command can't forbid itself, this shouldn't be able to fall off the end of the list.) Sorry it took so long. Thanks for the bug report. Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [groups] : ! More than one in group is error
Hi Rob, With your fix, it doesn't segfault now. But does it matter, to give the proper option name in error message. If yes, then the fix fails in that. e.g. when running *./toybox touch -d 12 -r f2 f1* it throws the error message as *touch: No 'r' with 't'*. I feel the error message is not explanatory here. it should give the proper option names in here. my patch had this fix. Pls give your inputs. regards, Ashwini On Fri, Dec 28, 2012 at 6:21 AM, Rob Landley wrote: > On 12/27/2012 11:06:37 AM, Felix Janda wrote: > >> When trying to implement the remaining options for "pwd" I could also test >> toybox's group option handling. >> >> The option string I wanted to use is: ">0LP[!LP]" >> (It's the same for "LP[!LP]".) >> >> With the current argparsing I get a segfault for "pwd -L -P". When >> enabling >> TOYBOX_DEBUG it says "pwd: trailing [!LP]". On the other hand when using >> Ashwini's suggestion with disabled TOYBOX_DEBUG everthing seems to work, >> whereas with enabled TOYBOX_DEBUG I get the same error message as before. >> > > Ok, this I can reproduce. Hmmm... > > Ah. You're right, the test was wrong. (It got the condition right, but > couldn't work out the correct error message due to & vs &&.) > > Testing for NULL just hid the problem, it didn't fix it. I checked in a > fix. > > Thanks, > > Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [groups] : ! More than one in group is error
On 12/27/2012 11:06:37 AM, Felix Janda wrote: When trying to implement the remaining options for "pwd" I could also test toybox's group option handling. The option string I wanted to use is: ">0LP[!LP]" (It's the same for "LP[!LP]".) With the current argparsing I get a segfault for "pwd -L -P". When enabling TOYBOX_DEBUG it says "pwd: trailing [!LP]". On the other hand when using Ashwini's suggestion with disabled TOYBOX_DEBUG everthing seems to work, whereas with enabled TOYBOX_DEBUG I get the same error message as before. Ok, this I can reproduce. Hmmm... Ah. You're right, the test was wrong. (It got the condition right, but couldn't work out the correct error message due to & vs &&.) Testing for NULL just hid the problem, it didn't fix it. I checked in a fix. Thanks, Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [groups] : ! More than one in group is error
When trying to implement the remaining options for "pwd" I could also test toybox's group option handling. The option string I wanted to use is: ">0LP[!LP]" (It's the same for "LP[!LP]".) With the current argparsing I get a segfault for "pwd -L -P". When enabling TOYBOX_DEBUG it says "pwd: trailing [!LP]". On the other hand when using Ashwini's suggestion with disabled TOYBOX_DEBUG everthing seems to work, whereas with enabled TOYBOX_DEBUG I get the same error message as before. Felix ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [groups] : ! More than one in group is error
On 12/26/2012 10:24:53 PM, Ashwini Sharma wrote:
Hey Rob,
Options mutual exclusion handling is having a Seg Fault in function
gotflag.
I think that means you referenced an option that doesn't exist. (If you
enable TOYBOX_DEBUG in the config, it's supposed to catch this and spit
out an "unknown target" message during parse_optflaglist().
What does your option string look like?
The block handling this doesn't have a terminating condition, hence
once it
goes beyond the options list, it crashes.
Because it should enver go beyond the options list because that means
the option you referenced wasn't found. This is why the [groups] are
supposed to go at the end of the options list.
"abc[!ac]" shouldn't segfault.
"abc[!ad]" won't find d and will fall off the list.
That said, I would have expected it to segfault on line 318
dereferencing a null opt there, not in gotflag().
At my end I have modified the code block from
if (toys.optflags & gof->excludes) {
struct opts *bad;
unsigned i =
1;
* for (bad=gof->opts; (gof->excludes && i; bad = bad->next) i<<=1;*
error_exit("No '%c' with '%c'", opt->c, bad->c);
}
to
if (toys.optflags & gof->excludes) {
struct opts *bad;
unsigned i =
1;
* gof->excludes &= ~opt->dex[1];
for (bad=gof->opts; bad && !(gof->excludes & i & toys.optflags) ;
bad =
bad->next) i<<=1;*
error_exit("No '%c' with '%c'", opt->c, bad->c);
}
Please have a look and add the same to the mainline.
I'd like to understand how it got there. Could you show me the option
string that triggered this behavior? I'd like to reproduce it so I
understand what actually went wrong.
Also, does switching on TOYBOX_DEBUG catch whatever is wrong with the
string? (If not, I need to add another test...)
Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
