Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: Fixed gem5img.py script #990

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

elatalhm
Copy link

@elatalhm elatalhm commented Apr 3, 2024

Made the script more robust to different names.

Copy link
Contributor

@giactra giactra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @elatalhm first of all thanks for this contribution. I have two comments (see below) which are mainly requesting to better document in the PR what is the problem and the solution you are providing

@@ -194,7 +194,8 @@ def findPartOffset(devFile, fileName, partition):
r"\s*:\s*" # Separator
r"start=\s*(?P<start>\d+),\s*" # Partition start record
r"size=\s*(?P<size>\d+),\s*" # Partition size record
r"type=(?P<type>\d+)" # Partition type record
r"type=(?P<type>.+?),*?" # Partition type record
r".*" # anything else, e.g., name field
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name field should be already covered (see bfbab75#diff-4cde24986c38a36e85628238a3e49f0732987ef0c7ec70c942ef179f5af6c578R193) unless you are telling me the sfdisk output sometimes rearranges its position.

Just to clarify, I am not oppsing the regex per se, I am wondering whether the comment is a bit misleading. For example I can see by playing a bit with sfdisk that I have an uuid entry so maybe that should be used as an example?

Copy link
Author

@elatalhm elatalhm Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this output on the x86-ubuntu.img file from gem5 resources:

$ sfdisk -d x86-ubuntu.img
label: dos
label-id: 0xd0c30ca2
device: x86-ubuntu.img
unit: sectors

x86-ubuntu.img1 : start=        2048, size=     4190208, type=83, bootable

The problem is caused by the , bootable at the end. The end of the regex has a \s*$ so it expects the line to end after the type field.

@@ -194,7 +194,8 @@ def findPartOffset(devFile, fileName, partition):
r"\s*:\s*" # Separator
r"start=\s*(?P<start>\d+),\s*" # Partition start record
r"size=\s*(?P<size>\d+),\s*" # Partition size record
r"type=(?P<type>\d+)" # Partition type record
r"type=(?P<type>.+?),*?" # Partition type record
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you specify on the PR/commit what's the problem with

(?P<type>\d+)

I presume you encountered some non digit types; I think it would be great if these example were documented in the PR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in response to an issue I was having with the x86 disk image from the gem5 resource page. I was getting the following output from calling ``sudo python3 util/gem5img.py mount x86-ubuntu.py mnt''

%> /usr/sbin/losetup /dev/loop20 x86-ubuntu.img
No partition description was found in sfdisk output:
label: dos
label-id: 0xeb7b110b
device: /dev/loop20
unit: sectors
sector-size: 512

/dev/loop20p1 : start= 2048, size= 4190208, type=83, bootable
Could not determine size of first partition.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type field can produce hexadecimal numbers. This can be verified just by running sfdisk -T to list the supported types.

@ivanaamit
Copy link
Contributor

Hi @elatalhm,

None of the commits in this pull request contains a Change-ID, which we require for any changes made to gem5. To automatically insert one, run the following:

f=.git/hooks/commit-msg
mkdir -p $f
curl -Lo $f https://gerrit-review.googlesource.com/tools/hooks/commit-msg
chmod +x $f

Then, amend the commit with git commit --amend --no-edit, and update your pull request. Thank you.

@ivanaamit
Copy link
Contributor

@elatalhm, could you please address Giacomo's comments and incorporate the requested changes? Thank you.

Made the script more robust to different names.

Change-Id: I2f428ed3fafc5339b07a5330693d35e06d8d5993
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants