On 04/23/11 23:49, nathan binkert wrote: >> I looked at it and it is similar, but it's different enough that they can't >> be interchanged. My functions all allow passing a string in as standard >> input and return the error code, but readCommand doesn't. I could extend it >> to take an input string, but it's not clear how to return the output and the >> error code without changing all the call sites and complicating all the code >> that's working fine now without it. readCommand doesn't allow printing the >> command before it's run so I'd have to add a wrapper to handle that, and it >> doesn't handle adding 'sudo'. I think tying in readCommand would actually >> make everything bigger and more complicated because it's not quite what I >> need and I'd have to add a bunch of wrappers. Also wrapping all the possible >> variations of Popen people might need (not that that's what you're saying, >> but it heads that way) is probably also counter productive because at that >> point you might as well just use Popen directly. >> >> > I totally agree. If it doesn't work, it doesn't work. Is it something > reusable that should go in m5.util? > > Nate > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev
I'm inclined to say no for two reasons. First, the whole sudo thing is still a little shady. The path thing is basically because sudo inherits the current environment, I think. You can pass -i to it to make it work like root just logged in and ran a command, but then that might clobber things people purposefully put in their environment like tweaking PATH. Appending /sbin and /usr/sbin is a workaround which has drawbacks which you've pointed out, but then forcing people to add them to their path when not root is also cumbersome. Running sudo automatically is probably not a universally good idea. So with all that sort of up in the air, I'm willing to let it slide for this one script because it's historically been that way and I don't have a better idea, but I don't want to make it institutional by putting it into a utility module. Second, there's some script specific plumbing here like the part where it prints out normally silent commands if you want to try to debug what's going on. You could make whether to print the command a parameter, but that over specializes the library functions, I think. Also, printing the command like I'm doing isn't quite right either. If you were to copy and paste one of the commands at a prompt (which is why they're frequently printed), it's conceivable it wouldn't work like it does in the script since there's no quoting of list elements with spaces in them. They get handled by Popen right because they're a single element in the list, but if you copy and paste the shell will split them up. All that said, if you want to take the script and move things into m5.util, or rework it so it uses m5.util, or make it handle paths better, or whatever, I certainly won't try to stop you. I think my version is an improvement over the original, but it's definitely not perfect. Gabe _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
