Skip to content

s390x: cleanups#1756

Merged
nikita-dubrovskii merged 3 commits into
coreos:mainfrom
nikita-dubrovskii:s390x_cleanups
Jul 2, 2026
Merged

s390x: cleanups#1756
nikita-dubrovskii merged 3 commits into
coreos:mainfrom
nikita-dubrovskii:s390x_cleanups

Conversation

@nikita-dubrovskii

Copy link
Copy Markdown
Contributor

See individual commits:

  • s390x/zipl: fix lszdev bus ID regex and add parser tests
  • d34f3 s390x: minor code cleanups in dasd and zipl handling
  • s390x/zipl: fix write() to write_all() in generate_initrd

Write::write() is not guaranteed to write all bytes; use write_all()
to avoid silent truncation of the appended initrd.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several s390x-specific improvements, including fixing a potential silent truncation issue by switching from write() to write_all() in initrd generation, improving UTF-8 validation when handling device paths, and refining the regular expressions used to parse lszdev bus IDs alongside adding corresponding unit tests. The review feedback suggests extracting the duplicated UTF-8 validation of dest_path into a local variable in dasd.rs, and trimming inputs and anchoring the regular expressions in zipl.rs to prevent matching malformed IDs with trailing characters.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/s390x/dasd.rs
Comment thread src/s390x/zipl.rs
Comment thread src/s390x/zipl.rs
- Use into_iter() instead of iter() when mapping owned Vec<String> to
  Vec<PathBuf> — moves each String into PathBuf rather than cloning
- Replace to_string_lossy().to_string() with to_str().with_context()
  to avoid double allocation and silent name mangling
- Escape literal dots, tighten devno to 1-4 hex digits, use [[:xdigit:]]
  consistently, switch captures_iter().next() to captures()
- Add tests for parse_lszdev_eckd and parse_lszdev_zfcp
@nikita-dubrovskii

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several improvements and bug fixes for s390x, including switching from write() to write_all() in initrd generation to prevent silent truncation, refining the regex patterns for parsing lszdev bus IDs (specifically escaping dots and anchoring the patterns), and adding comprehensive unit tests for these parsers. Additionally, it refactors some path handling in dasd.rs to use UTF-8 string slices instead of lossy conversions. The review feedback suggests replacing the lazy_static! macro with the standard library's std::sync::LazyLock in the regex definitions to simplify the code and reduce external dependencies.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/s390x/zipl.rs
Comment thread src/s390x/zipl.rs

@jbtrystram jbtrystram left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@nikita-dubrovskii nikita-dubrovskii merged commit 431a120 into coreos:main Jul 2, 2026
15 checks passed
@nikita-dubrovskii nikita-dubrovskii deleted the s390x_cleanups branch July 2, 2026 08:59
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