Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/644/ --- (Updated 2011-04-24 03:48:03.724082) 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 (updated) - 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
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
This new version adds a % prompt in front of echoed commands to make them more obvious, cleans up some minor redundancy in one of the functions, replaces the needSudo singleton with a simpler function, and now measures the offset of the first partition in the image instead of hard coding it. The hard coded value would be correct for images above a certain size where the number of sectors saturates at 63 and the DOS compatibility region is in place, but may not be correct otherwise. Gabe On 04/24/11 06:48, Gabe Black wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/644/ --- (Updated 2011-04-24 03:48:03.724082) 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 (updated) - 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 ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
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. Nathan Binkert wrote: 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. 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. - 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
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
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
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
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
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
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. I didn't mean the whole thing. I meant stuff that might be generally useful. Nate ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
On 04/24/11 00:14, nathan binkert wrote: 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. I didn't mean the whole thing. I meant stuff that might be generally useful. Nate ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev Yes, that's what I'm talking about too. Gabe ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
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
[m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/644/ --- 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
Re: [m5-dev] Review Request: Util: Replace mkblankimage.sh with the new gem5img.py.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/644/#review1133 --- util/gem5img.py http://reviews.m5sim.org/r/644/#comment1542 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.) util/gem5img.py http://reviews.m5sim.org/r/644/#comment1543 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 util/gem5img.py http://reviews.m5sim.org/r/644/#comment1544 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 util/gem5img.py http://reviews.m5sim.org/r/644/#comment1545 This is basically readCommand() util/gem5img.py http://reviews.m5sim.org/r/644/#comment1546 Here is where you could suggest that it is in /sbin or /usr/sbin - Nathan 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
