-
Notifications
You must be signed in to change notification settings - Fork 290
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
repo: Add an API to get min-free-space-* reserved bytes #1715
Conversation
c9f2da2
to
43900c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments. I’d be pretty keen to see a unit test for this too, otherwise there’s zero code in libostree.git which actually calls this new function.
43900c7
to
d5aab1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this would really benefit from a unit test.
src/libostree/ostree-repo-commit.c
Outdated
if (self->reserved_blocks > G_MAXUINT64 / self->txn.blocksize) | ||
{ | ||
g_mutex_unlock (&self->txn_lock); | ||
return glnx_throw (error, "min-free-space value(in bytes) was too high"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to include the maximum permitted value in this error message. For example:
return glnx_throw ("min-free-space value is greater than the maximum allowed value of %" G_GUINT64_FORMAT " bytes", G_MAXUINT64 / self->txn.blocksize);
src/libostree/ostree-repo.c
Outdated
* If the configuration value of min-free-space-* value is too high(overflow) | ||
* then @error is set with status code %G_IO_ERROR_FAILED. | ||
* | ||
* Returns: Value(in bytes) of min-free-space-* config option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Put a space before (
here and in the paragraph above.
src/libostree/ostree-repo.c
Outdated
|
||
g_assert (self->txn.blocksize != 0); | ||
if (self->reserved_blocks > G_MAXUINT64 / self->txn.blocksize) | ||
return glnx_throw (error, "min-free-space(in bytes) value was too high"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Make sure this error message is the same as whatever you change the one above to be.)
ae8c3eb
to
cadfb9b
Compare
tests/test-repo.c
Outdated
const char *values_to_test[] = { "500MB", | ||
"0MB", | ||
// "17179869185GB", /* Overflow parameter: bytes > G_MAXUINT64 */ | ||
// "17179869183GB", /* bytes approx. under G_MAXUINT64 */ //TODO: Bypass failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do both of these values error out in ostree_repo_prepare_transaction()
? If so, that’s probably the best testing we’re going to get, and there’s no point in coming up with contortions to get more coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they both error out in ostree_repo_prepare_transaction
with different appropriate error messages.
If so, that’s probably the best testing we’re going to get, and there’s no point in coming up with > contortions to get more coverage.
Can you explain this a bit more? Does it mean there is no need for 0MB
, 500MB
test etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in, don’t try and make the prepare_transaction()
call artificially succeed so you can test those large values with the get_min_free_space_bytes()
call.
src/libostree/ostree-repo.c
Outdated
g_return_val_if_fail (self->in_transaction == TRUE, 0); | ||
|
||
g_assert (self->txn.blocksize != 0); | ||
if (self->reserved_blocks > G_MAXUINT64 / self->txn.blocksize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pwithnall So I was asking about this check. Do we check(again) for overflows here when min_free_space_calculate_reserved_blocks()
already does it before setting self->reserved_blocks
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calculate_reserved_blocks()
does a different check, though; it checks for overflow with a << 20
shift. You should only remove the check here if you can statically guarantee that if << 20
won’t overflow, then * self->txn.blocksize
won’t overflow. You can do that by checking in prepare_transaction()
that self->txn.blocksize < 2^20
. That would be a good change to make, because you could then drop the GError
argument from get_min_free_space_bytes()
.
cd17970
to
2a188fc
Compare
when converted to bytes In a subsequent commit, we add a public API to read the value of min-free-space-* value in bytes. The value for free space check is enforced in terms of block size instead of bytes. Therefore, for consistency we check while preparing the transaction that the value doesn't overflow when converted to bytes. https://phabricator.endlessm.com/T23694
2a188fc
to
d056bbd
Compare
tests/test-repo.c
Outdated
|
||
config = ostree_repo_copy_config (repo); | ||
|
||
while (values_to_test[i] != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Use a for
loop for this. That saves you having to manually increment i
on every loop path below.
i.e. for (i = 0; values_to_test[i] != NULL; i++)
d056bbd
to
1bb9bb0
Compare
@cgwalters @jlebon Can you take a look ? it's LGTM from @pwithnall's side now. |
tests/test-repo.c
Outdated
const char *values_to_test[] = { "500MB", | ||
"0MB", | ||
"17179869185GB", /* Overflow parameter: bytes > G_MAXUINT64 */ | ||
NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be slightly cleaner I think to have this be an array of 2-tuple (struct) { const char *val; bool should_succeeed }
and assert that error
is set if and only if should_succeed
.
That said, up to you, I don't mind merging as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion.
Pushed a fixup
commit.
|
when converted to bytes In a subsequent commit, we add a public API to read the value of min-free-space-* value in bytes. The value for free space check is enforced in terms of block size instead of bytes. Therefore, for consistency we check while preparing the transaction that the value doesn't overflow when converted to bytes. https://phabricator.endlessm.com/T23694 Closes: #1715 Approved by: cgwalters
https://phabricator.endlessm.com/T23694 Closes: #1715 Approved by: cgwalters
https://phabricator.endlessm.com/T23694 Closes: #1715 Approved by: cgwalters
💔 Test failed - status-atomicjenkins |
Looks like a flake.
@rh-atomic-bot retry
|
https://phabricator.endlessm.com/T23694 Closes: #1715 Approved by: cgwalters
https://phabricator.endlessm.com/T23694 Closes: #1715 Approved by: cgwalters
☀️ Test successful - status-atomicjenkins |
…onverted to bytes In a subsequent commit, we add a public API to read the value of min-free-space-* value in bytes. The value for free space check is enforced in terms of block size instead of bytes. Therefore, for consistency we check while preparing the transaction that the value doesn't overflow when converted to bytes. https://phabricator.endlessm.com/T23694 Closes: ostreedev#1715 Approved by: cgwalters (cherry-picked from 3814d07)
https://phabricator.endlessm.com/T23694 Closes: ostreedev#1715 Approved by: cgwalters (cherry-picked from 68420f7)
https://phabricator.endlessm.com/T23694 Closes: ostreedev#1715 Approved by: cgwalters (cherry-picked from a70d2f6)
https://phabricator.endlessm.com/T23694