Skip to content

Add image string information to the display output of the fw-info subcommand #339

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

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

Conversation

hsinliang
Copy link

Add image string information to the display output of the fw-info subcommand. The user can modify the image string in the firmware header using the ChipLink Firmware Image Header Editor tool. After updating the firmware, use Switchtec-User fw-info to verify the image string information.

{
int i;

for (i = 0; i < len; i++)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please follow the coding style: should use tab characters (set to 8 spaces) as indents and the { for for and if statements should be on the same line.

This project follows Linux coding style so see here for more information:
https://www.kernel.org/doc/html/latest/process/coding-style.html


if(inf->valid){
printf("Image String:");
char_arr_print(inf->img_str, sizeof(inf->img_str));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't actually need the new helper, it should be equivalent to:

printf("%.*s", sizeof(inf->img_str), inf->img_str);

tag, inf->version, inf->image_crc,
inf->read_only ? "(RO)" : "",
inf->running ? " (Running)" : "",
inf->valid ? "" : " (Invalid)");

if(inf->valid){
printf("Image String:");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The lines here are going to be too long for a standard screen. Can we maybe add this to a new line and match the indent of the Version string?

if (inf->valid) {
    printf("  %-4s\tImage String: %.*s\n", "", sizeof(inf->img_str), 
              inf->img_str);

@@ -259,6 +259,7 @@ struct switchtec_fw_image_info {
size_t part_body_offset; //!< Partition image body offset
size_t image_len; //!< Length of the image
unsigned long image_crc; //!< CRC checksum of the image
char img_str[16];
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: I'd very much prefer changes to the library be put in separate patches from changes to the CLI.

@@ -1095,6 +1096,9 @@ static int switchtec_fw_info_metadata_gen5(struct switchtec_dev *dev,
inf->image_len = le32toh(metadata->image_len);
inf->metadata = metadata;

for(i = 0; i< 16; i++)
inf->img_str[i] = metadata->img_str[i];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be a memcpy() and shouldn't need the constant 16:

memcpy(inf->img_str, metadata->img_str, sizeof(inf->img_str));

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.

2 participants