/snap-confine

Add error support code #199

Merged
merged 8 commits into from Dec 5, 2016

Conversation

Reviewers

@chipaca chipaca

@vosst vosst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zyga @vosst @tyhicks @chipaca
@zyga
Collaborator

zyga commented Dec 1, 2016

This patch adds die()-like APIs that can carry error information around
the control flow. They are meant to provide richer error information in
cases where the error is not fatal and needs to be returned to the
caller. The APIs are 100% tested and also integrate with die() where
appropriate().

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com
@zyga
Add error support code 
This patch adds die()-like APIs that can carry error information around
the control flow. They are meant to provide richer error information  in
cases where the error is not fatal and needs to be returned to the
caller. The APIs are 100% tested and also integrate with die() where
appropriate().

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
287867e
@vosst

vosst reviewed Dec 1, 2016
View changes

Code is looking good overall. I left a couple of comments inline, mostly nits.

src/error.h
+ * This function calls die() in case of memory allocation failure.
+ **/
+__attribute__ ((format(printf, 3, 4)))
+struct sc_error *sc_error_init(const char *domain, int code, const char *msgfmt,
@vosst

vosst Dec 1, 2016

Contributor

nit: _new or _alloc might go better with _free.

@chipaca

chipaca Dec 2, 2016

Member

also, consider attributes nonnull (unlless format wanrs about a null msgfmt already), warn_unused_result, returns_nonnull.

src/error.h
+/**
+ * Get the error domain out of an error object.
+ *
+ * The error domain acts as a namespace for error codes. The return value must
@vosst

vosst Dec 1, 2016

Contributor

nit: I would propose to rephrase the doc string a little bit: "No change of ownership takes place".

@zyga

zyga Dec 2, 2016

Collaborator

Hmm, this feels C++-ish but as long as we're consistent that's okay. Changed

src/error.h
+/**
+ * Get the error message out of an error object.
+ *
+ * The error message is bound to the life-cycle of the error object.
@vosst

vosst Dec 1, 2016

Contributor

See previous comment on ownership.

@zyga

zyga Dec 2, 2016

Collaborator

Changed, thanks.

src/error.h
+ * The error message is derived from the data in the error, using the special
+ * errno domain to provide additional information if that is available.
+ **/
+void sc_error_die(struct sc_error *error);
@vosst

vosst Dec 1, 2016

Contributor

Maybe rename to sc_error_die_if.

@chipaca

chipaca Dec 2, 2016

Member

or sc_error_die_maybe |-)

@zyga

zyga Dec 2, 2016

Collaborator

I called this sc_die_on_error to make it dead obvious the cost of the naming scheme.

src/error.h
+ * the caller did not provide a location for the error to be stored then the
+ * sc_error_die() is called as a safety measure.
+ **/
+void sc_error_forward(struct sc_error **recepient, struct sc_error *error);
@vosst

vosst Dec 1, 2016

Contributor

I'm slightly confused by this method (likely because I lack a bit of context): I would have expected sc_error* sc_error_forward_or_die(struct sc_error* error);, with the function dying if error is NULL.

@zyga

zyga Dec 1, 2016

Collaborator

This is essentially a way to enforce a contract that an error is forwarded to the caller or we die right here, right now. For usage context look at https://github.com/snapcore/snap-confine/blob/arguments/src/snap-confine-args.c#L135

@chipaca

chipaca Dec 2, 2016

Member

here a nonnull for the recepient would be good i think

@zyga
Collaborator

zyga commented Dec 1, 2016

Thanks for the review. I'll fix something in another branch and return to this quickly.
@chipaca

chipaca reviewed Dec 2, 2016
View changes

It looks good. I've pointed out some attributes you could use to helper the compiler errors/warnings, but those are pico-nits.

One thing that does bother me a little is that you pay for formatting things up front instead of at die time. Had you considered carrying around the msgfmt and va_list and doing the vasprintf at the end if needed?

src/error.h
+ * This function calls die() in case of memory allocation failure.
+ **/
+__attribute__ ((format(printf, 3, 4)))
+struct sc_error *sc_error_init(const char *domain, int code, const char *msgfmt,
@vosst

vosst Dec 1, 2016

Contributor

nit: _new or _alloc might go better with _free.

@chipaca

chipaca Dec 2, 2016

Member

also, consider attributes nonnull (unlless format wanrs about a null msgfmt already), warn_unused_result, returns_nonnull.

src/error.h
+ *
+ * This function calls die() in case of memory allocation failure.
+ **/
+__attribute__ ((format(printf, 2, 3)))
@chipaca

chipaca Dec 2, 2016

Member

maybe some of those apply here as well

src/error.h
+ * The error domain acts as a namespace for error codes. The return value must
+ * not be released in any way.
+ **/
+const char *sc_error_domain(struct sc_error *err);
@chipaca

chipaca Dec 2, 2016

Member

and all of these could probably benefit from nonnull

src/error.h
+ * The error message is derived from the data in the error, using the special
+ * errno domain to provide additional information if that is available.
+ **/
+void sc_error_die(struct sc_error *error);
@vosst

vosst Dec 1, 2016

Contributor

Maybe rename to sc_error_die_if.

@chipaca

chipaca Dec 2, 2016

Member

or sc_error_die_maybe |-)

@zyga

zyga Dec 2, 2016

Collaborator

I called this sc_die_on_error to make it dead obvious the cost of the naming scheme.

src/error.h
+ * the caller did not provide a location for the error to be stored then the
+ * sc_error_die() is called as a safety measure.
+ **/
+void sc_error_forward(struct sc_error **recepient, struct sc_error *error);
@vosst

vosst Dec 1, 2016

Contributor

I'm slightly confused by this method (likely because I lack a bit of context): I would have expected sc_error* sc_error_forward_or_die(struct sc_error* error);, with the function dying if error is NULL.

@zyga

zyga Dec 1, 2016

Collaborator

This is essentially a way to enforce a contract that an error is forwarded to the caller or we die right here, right now. For usage context look at https://github.com/snapcore/snap-confine/blob/arguments/src/snap-confine-args.c#L135

@chipaca

chipaca Dec 2, 2016

Member

here a nonnull for the recepient would be good i think

@zyga
Add support for matching errors 
This patch adds a function for checking if a given error (or no error
perhaps) matches a given domain and code. It is better than doing this
manually each time.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
f91a3d3
@zyga
Collaborator

zyga commented Dec 2, 2016

@chipaca In C that's generally hard as va_list is hard to "store" for longer than a function call and even if you could then there's no guarantee that value stored there are still sane (e.g. pointers). One of the non-perks of manual memory management :/

zyga added some commits Dec 2, 2016

@zyga
Add handful of function attributes 
The test code got a little bit more ugly to ensure that the compiler is
happy but I think it is worth it anyway.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
0bffb3c
@zyga
Rename sc_error_die to sc_die_on_error 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
5f5b98e
@zyga
Tweak documentation and fix typo 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
0b50a35
@zyga
Nullify err->msg when freeing 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
60efbdf
@zyga
Use functions to access error fields 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
5929ec2
@zyga
Allow sc_error_forward recipient to be NULL 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
9265148
@vosst
Contributor

vosst commented Dec 2, 2016

LGTM.

@zyga zyga merged commit 9227986 into master Dec 5, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zyga zyga deleted the errors branch Dec 5, 2016

@tyhicks
Collaborator

tyhicks commented Dec 5, 2016

I took a quick look at this PR and it looks ok to me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment