Add a function to re-read size of the loop device#1126
Conversation
Summary of ChangesHello @vojtechtrefny, 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 introduces a critical enhancement to 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new function bd_loop_set_capacity to allow re-reading the size of a loop device, which is useful when the backing file has been resized. The implementation includes the core logic, API definitions, documentation updates, and a new test case.
My review has identified a bug in the progress message formatting that could lead to incorrect paths in log messages. I've also provided suggestions to improve code readability in the C implementation and reduce code duplication in the Python test suite. Overall, the changes are well-structured and the new functionality is a valuable addition.
9df1103 to
f191bf8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new function, bd_loop_set_capacity, which allows forcing a loop device to re-read the size of its backing file. The implementation is sound, and it's accompanied by good documentation and a comprehensive test case. The changes are well-structured. I have one suggestion to improve code readability in the C implementation.
| for (n_try=10, status=-1; (status != 0) && (n_try > 0); n_try--) { | ||
| status = ioctl (fd, LOOP_SET_CAPACITY, 0); | ||
| if (status < 0 && errno == EAGAIN) | ||
| g_usleep (100 * 1000); /* microseconds */ | ||
| else | ||
| break; | ||
| } |
There was a problem hiding this comment.
The logic in this for loop is a bit dense due to the combined initialization and condition, along with the if/else break structure. A more conventional loop structure would be more readable and easier for future maintenance.
for (n_try = 10; n_try > 0; n_try--) {
status = ioctl (fd, LOOP_SET_CAPACITY, 0);
if (status == 0)
break;
if (errno != EAGAIN)
break;
g_usleep (100 * 1000); /* microseconds */
}| for (n_try=10, status=-1; (status != 0) && (n_try > 0); n_try--) { | ||
| status = ioctl (fd, LOOP_SET_CAPACITY, 0); | ||
| if (status < 0 && errno == EAGAIN) | ||
| g_usleep (100 * 1000); /* microseconds */ | ||
| else | ||
| break; | ||
| } |
| g_free (dev_loop); | ||
| if (fd < 0) { | ||
| g_set_error (&l_error, BD_LOOP_ERROR, BD_LOOP_ERROR_DEVICE, | ||
| "Failed to open device %s: %m", loop); |
There was a problem hiding this comment.
So previously you use a complex construct like dev_loop ? dev_loop : loop for messages but in the rest of the code you just use loop... I don't mind, I wonder why the AI didn't spot this.
There was a problem hiding this comment.
Because I copied it from two different functions with two different messages :-)
24f99d3
into
storaged-project:master
No description provided.