kdump.sh: Strip the surrounding quotes from configuration values #122
Conversation
Summary of ChangesHello @licliu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
commit 1a73387(Strip surrounding quotes from configuration values) omitted modifications to kdump.sh. Adds a POSIX-compliant `case` statement to remove the surrounding quotes. Assisted-by: Google Gemini Signed-off-by: Lichen Liu <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds logic to kdump.sh to strip surrounding quotes from configuration values, which was missed in a previous commit. The approach of using a POSIX-compliant case statement is appropriate for this shell script. However, the implementation has a bug in the pattern matching that could lead to incorrect parsing of configuration values. My review includes a suggestion to fix this bug and also to make the quote stripping more robust by handling single quotes.
prudo1
left a comment
prudo1
left a comment
There was a problem hiding this comment.
Hi Lichen,
true we missed this one... but we also missed an other one in mkdumprd...
That makes me think that it's probably better to remove the quotes in kdump_read_conf rather than in all its users. Although would probably mean to rewrite the function as adding it to the sed expression would most likely make it completely unreadable...
What do you think?
Thanks
Philipp
|
Hi Lichen,
Anyway, I've pushed the changes to a branch on my fork in case you want to take a look at it. Thanks |
|
Hi @prudo1, thanks for reviewing this! I had the same thought before submitting, and I agree that your suggestion is the correct approach. The main reason I didn't implement it was my lack of confidence with regex. I needed to use an AI tool just to understand the current expression, as it was too long for me to verify manually. Admittedly, this is in my current sprint, so I was hoping for a quick merge. However, you're right that my current implementation could cause bugs down the line. Your approach makes more sense. I took a quick look at your code—since it's marked WIP, I didn't dive deep into the details to see if there are problems. It makes the whole flow much clearer. I also felt before that kdump_read_conf and kdump_get_conf_val were doing redundant work, and it's been bothering me for a while. I'm planning to remove this issue from my current sprint for now. To avoid duplicating our work, would you be willing to finish this patch? Thanks, |
2 participants
commit 1a73387(Strip surrounding quotes from configuration values) omitted modifications to kdump.sh.
Adds a POSIX-compliant
casestatement to remove the surrounding quotes.