Add sonic_installer utility. #40
Conversation
|
@qiluo-msft , can you review this? |
sonic_installer/main.py
Outdated
|
|
||
| MACHINE_PATH = '/host' | ||
| IMAGE_PREFIX = 'SONiC-OS' | ||
| IMAGE_DIR_PREFIX = 'image' |
There was a problem hiding this comment.
To be more strict:
IMAGE_PREFIX = 'SONiC-OS-'
IMAGE_DIR_PREFIX = 'image-' #Closed
There was a problem hiding this comment.
Done #Closed
sonic_installer/main.py
Outdated
| for line in config: | ||
| if line.startswith('menuentry'): | ||
| image = line.split()[1].strip("'") | ||
| if image != 'ONIE': |
There was a problem hiding this comment.
image != 'ONIE' [](start = 15, length = 15)
check for sonic explicitly? perhaps check IMAGE_PREFIX. #Closed
There was a problem hiding this comment.
Done #Closed
sonic_installer/main.py
Outdated
| config.close() | ||
|
|
||
| image_dir = image.replace(IMAGE_PREFIX, IMAGE_DIR_PREFIX) | ||
| run_command('rm -rf ' + MACHINE_PATH + '/' + image_dir) |
There was a problem hiding this comment.
run_command [](start = 4, length = 11)
shutil.rmtree is safer.
There was a problem hiding this comment.
In what way?
There was a problem hiding this comment.
Concatenating command line like 'rm -rf' is always dangerous and error prone. To prevent any usage error or developer extending errors (in future), let's use some API and relying upon it's argument validation.
In reply to: 113193343 [](ancestors = 113193343)
There was a problem hiding this comment.
I share the concern, but it seems the real danger comes from image_dir validation, if image_dir is null string, we could remove the whole /host/ directory. rmtree won't realy help here.
There was a problem hiding this comment.
it is better to validate the image_dir, make sure it is not null.
There was a problem hiding this comment.
image_dir may contain blank, ';', '&&'. For example:
rm -rf /host/image_a /
rm -rf /host/image_a; echo “hello world"
There was a problem hiding this comment.
Changed to rmtree
sonic_installer/main.py
Outdated
| else: | ||
| image_path = url | ||
|
|
||
| run_command("sh " + image_path) |
There was a problem hiding this comment.
run_command [](start = 4, length = 11)
Can we remove the explicit "sh"? The image should be executable and the shebang could be arbitrary. #Closed
There was a problem hiding this comment.
Done #Closed
sonic_installer/main.py
Outdated
| import urllib | ||
| import subprocess | ||
|
|
||
| MACHINE_PATH = '/host' |
sonic_installer/main.py
Outdated
| if line.find(image_dir): | ||
| current = image | ||
| break | ||
| cmdline.close() |
There was a problem hiding this comment.
use m=re.match("loop=(\S+)/fs.squash").
m.group(1) to get the image dir.
sonic_installer/main.py
Outdated
| entry_found = True | ||
| elif entry_found and line.startswith('}'): | ||
| click.echo('FOUND!!!') | ||
| entry_found = False |
There was a problem hiding this comment.
can we use regex instead of this simple state machine to find out the menuentry and remove it?
sonic_installer/main.py
Outdated
| click.echo('FOUND!!!') | ||
| entry_found = False | ||
| elif not entry_found: | ||
| new_config += line |
There was a problem hiding this comment.
add a comment, # remove menuentry of the image in grub.cfg
Fix constants Remove swap file added by mistake Signed-off-by: marian-pritsak <marianp@mellanox.com>
Its needed for sonic-net/sonic-utilities#40 for user to set default image for boot. grub-set-default utility writes to value of saved_entry variable to grubenv. https://www.gnu.org/software/grub/manual/legacy/Invoking-grub_002dset_002ddefault.html This patch provides support for grub-set-default to allow user choose a default image to boot from. Signed-off-by: marian-pritsak <marianp@mellanox.com>
|
I confirmed most my comments are resolved. Only one left: |
Signed-off-by: marian-pritsak <marianp@mellanox.com>
|
@qiluo-msft done requested changes, please review |
No description provided.