> On 2011-04-18 21:20:49, Nathan Binkert wrote: > > util/gem5img.py, line 51 > > <http://reviews.m5sim.org/r/644/diff/1/?file=11664#file11664line51> > > > > I'm not going to make you change it or anything, but this whole class > > seems to me to be a bit overkill, no? > > > > __notRoot = None > > def needSudo(): > > if __notRoot is None: > > __notRoot = os.geteuid() != 0 > > return __notRoot > > > > BTW: we also have m5.util.Singleton > > Gabe Black wrote: > Yes and no. This came about because the original script had a warning > about using sudo which I wanted to preserve. I made it possible to run only > part of the script, and if you run a part that doesn't need sudo (like just > creating an appropriately sized file) then the warning might be confusing. I > also didn't want the warning to show up multiple times if sudo was used more > than once. It's a little clumsy, but I didn't see any obvious alternative > that would get the behavior I wanted. Feel free to convince me to drop the > warning.
You can add the error message to the code that I pasted above. I was just saying that the singleton was a bit much and just serves to confuse things. The _notRoot thing is probably overkill too. Since this is not performance code, no reason not to just check geteuid() each time. None of this matters much though. > On 2011-04-18 21:20:49, Nathan Binkert wrote: > > util/gem5img.py, line 64 > > <http://reviews.m5sim.org/r/644/diff/1/?file=11664#file11664line64> > > > > This is pretty similar to m5.util.readCommand which made me think that > > it might be nice if we put your utility functions here in m5.util > > Gabe Black wrote: > Can I use m5.util from an arbitrary python script? If I can that's good > to know. Also, how does readCommand work? Does it pass through stdout/stderr > or capture it? Depending on the answer it might be an appropriate replacement > for this or the subsequent getOutput, but changing only one obscures the > similarities between the two functions. If you're advocating adding a new > version of readCommand that has the other behavior then that makes sense. I > also funnel text into stdin for input, and I think sudo happens to still work > because it goes around any redirection I set up. Yes, you can. Several scripts do that. You have to get it into your path though. Check out the style hook. I suggest just looking at readCommand and the examples to see exactly what it does. > On 2011-04-18 21:20:49, Nathan Binkert wrote: > > util/gem5img.py, line 106 > > <http://reviews.m5sim.org/r/644/diff/1/?file=11664#file11664line106> > > > > Here is where you could suggest that it is in /sbin or /usr/sbin > > Gabe Black wrote: > I'm not sure what telling them where it might be would accomplish since > the script still wouldn't be able to find/use it. I may just not understand > what you're getting at. I'm simply saying that changing the user's path to add other locations to search seems sketchy to me. Perhaps for /usr/sbin and /sbin, it's ok though and I'm just being paranoid. > On 2011-04-18 21:20:49, Nathan Binkert wrote: > > util/gem5img.py, line 25 > > <http://reviews.m5sim.org/r/644/diff/1/?file=11664#file11664line25> > > > > This makes me feel uneasy for a script that you're likely to call using > > sudo. I know it's overly paranoid, but why not just simply give the user a > > tip if the program is not found (which you have to deal with anyway.) > > Gabe Black wrote: > I'm not necessarily advocating doing things this way, but this is what > the original script was doing. This script should not be called with sudo > since it calls it internally, although I suppose it could. I don't know why, > but I think if you use sudo to like the script does, you have to make sure > you use an absolute path to the target binary. There were some posts to back > that up online and that's also what the original script was doing. To get > that path, the script calls "which", and for that to find things that are > normally only usable by root it needs to also search in /sbin and /usr/sbin. > If a program isn't found the script will complain and die. I guess I think complain and die is the right thing to do. I don't like the idea of any script screwing with my path. I guess that's just my paranoia. - Nathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/644/#review1133 ----------------------------------------------------------- On 2011-04-18 02:37:48, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/644/ > ----------------------------------------------------------- > > (Updated 2011-04-18 02:37:48) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > Util: Replace mkblankimage.sh with the new gem5img.py. > > This change replaces the mkblankimage.sh script, used for creating new disk > images, with a new gem5img.py script. The new version is written in python > instead of bash, takes its parameters from command line arguments instead of > prompting for them, and finds a free loopback device dynamically instead of > hardcoding /dev/loop1. The file system used is now optionally configurable, > and the blank image is filled by a "hole" left by lseek and write instead of > literally filling it with zeroes. > > The functionality of the new script is broken into subcommands "init", > "mount", "umount", "new", "partition", and "format". "init" creates a new file > of the appropriate size, partitions it, and then formats the first (and only) > new parition. "mount" attaches a new loopback device to the first parition of > the image file and mounts it to the specified mount point. "umount" unmounts > the specified mount point and identifies and cleans up the underlying loopback > device. "new", "partition", and "format" are the individual stages of "init" > but broken out so they can be run individually. That's so an image can be > reinitialized in place if needed. > > Two features of the original script are being dropped. The first is the > ability to specify a source directory to copy into the new file system. The > second is the ability to specify a list of commands to run which are expected > to (but not required to) update the permissions of the files in the new fs. > Both of these seem easy enough to do manually, especially given the "mount" > and "umount" commands, that removing them would meaningfully simplify the > script without making it less useful. > > > Diffs > ----- > > util/gem5img.py PRE-CREATION > util/mkblankimage.sh d8ec0a7b3f0c > > Diff: http://reviews.m5sim.org/r/644/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
