Util: Replace mkblankimage.sh with the new gem5img.py.

Information
Submitter:	Gabe Black
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

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.

Testing Done

util/gem5img.py (Diff revision 1)

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 April 18, 2011, 4:52 p.m. (April 18, 2011, 4:52 p.m.)

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.

Nathan Binkert April 21, 2011, 6:59 a.m. (April 21, 2011, 6:59 a.m.)

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.

util/gem5img.py (Diff revision 1)

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 April 18, 2011, 4:55 p.m. (April 18, 2011, 4:55 p.m.)

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.

Nathan Binkert April 21, 2011, 6:57 a.m. (April 21, 2011, 6:57 a.m.)

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.

util/gem5img.py (Diff revision 1)

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 April 18, 2011, 5 p.m. (April 18, 2011, 5 p.m.)

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.

Nathan Binkert April 21, 2011, 6:58 a.m. (April 21, 2011, 6:58 a.m.)

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.

Gabe Black April 23, 2011, 1:43 p.m. (April 23, 2011, 1:43 p.m.)

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.

util/gem5img.py (Diff revision 1)

This is basically readCommand()

Gabe Black April 18, 2011, 5 p.m. (April 18, 2011, 5 p.m.)
```
See above.
```

util/gem5img.py (Diff revision 1)

Here is where you could suggest that it is in /sbin or /usr/sbin

Gabe Black April 18, 2011, 5:01 p.m. (April 18, 2011, 5:01 p.m.)

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.

Nathan Binkert April 21, 2011, 6:59 a.m. (April 21, 2011, 6:59 a.m.)

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.

Diff:

Revision 2 (+372 -219)

Show changes

	util/gem5img.py
	util/mkblankimage.sh

Diff:

Revision 3 (+372 -219)

Show changes

	util/gem5img.py
	util/mkblankimage.sh

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff:

Diff: