> 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.)
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. > 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 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. > 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 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. > On 2011-04-18 21:20:49, Nathan Binkert wrote: > > util/gem5img.py, line 72 > > <http://reviews.m5sim.org/r/644/diff/1/?file=11664#file11664line72> > > > > This is basically readCommand() See above. > 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 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. - Gabe ----------------------------------------------------------- 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
