-
Notifications
You must be signed in to change notification settings - Fork 238
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
bind_mount: Return an error code, and provide a way to display it #434
Conversation
02de2ff
to
4f2d26f
Compare
Now updated after merging #403. |
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.
LGTM as is - please consider my comments as optionals. You can merge as is or do a followup or not.
bind-mount.c
Outdated
case BIND_MOUNT_ERROR_MOUNT: | ||
fprintf (stderr, "Unable to mount source on destination"); |
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 think this would be more elegant with a helper method equivalent of strerror()
- i.e. it returns a const char *
instead of doing the printing itself. (Or analogus to impl Display for BindError
in Rust).
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.
One problem with doing this is that if we get an unknown/invalid bind_mount_error
, we can't fall back to printing it with %d
, unless the helper method returns a newly-allocated char *
(or a static buffer, I suppose, but eww).
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.
New version adds bind_mount_result_to_string()
, does that look better to you?
|
||
case BIND_MOUNT_ERROR_FIND_DEST_MOUNT: | ||
fprintf (stderr, "Unable to find destination in mount table"); | ||
want_errno = FALSE; |
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.
Though I guess this is a wrinkle so it'd need to return two values. (Or...maybe just reset errno = 0
and don't print it if zero?)
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'm already not a fan of the "action at a distance" API that is errno
, so having helper functions edit errno
as a side-effect seems nasty to me.
I was half-tempted to reinvent the GError
/DBusError
pattern here, like I did in libcapsule, but held off on doing that right now.
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 went for "return two values".
This is a useful way to detect bugs before they happen. Signed-off-by: Simon McVittie <smcv@collabora.com>
I think I'd like to land this as-is to get the better reporting, and then try refactoring as a followup and see whether it looks like a net improvement. |
Hmm, actually, I should redo this, because |
This makes it clearer that we have thought about whether they need symlinks for source resolved, and concluded that they do not. Signed-off-by: Simon McVittie <smcv@collabora.com>
This is a useful way to catch possibilities being unhandled. Signed-off-by: Simon McVittie <smcv@collabora.com>
This gives us better diagnostic messages on failure, particularly for BIND_MOUNT_ERROR_FIND_DEST_MOUNT where we previously said "Invalid argument". Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
build: Error if a switch lacks a default case
This is a useful way to detect bugs before they happen.
Handle all enum values in switch
This makes it clearer that we have thought about whether they need
symlinks for source resolved, and come to the conclusion that they do not.
build: Warn if a switch over an enum does not handle all values
This is a useful way to catch possibilities being unhandled.
bind_mount: Return an error code, and provide a way to display it
This gives us better diagnostic messages on failure, particularly for
BIND_MOUNT_ERROR_FIND_DEST_MOUNT where we previously said "Invalid
argument".
bind-mount: Factor out bind_mount_result_to_string()