Skip to content
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

WIP: add sml_error function pointer, allow user to intercept error messages #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

r00t-
Copy link
Collaborator

@r00t- r00t- commented Dec 28, 2020

a basic attempt at fixing #17 .

replaces hardcoded fprintf() calls with an sml_error function pointer that can be overwritten by code using the library.
a default is provided that mimics the old behaviour (write to stderr with a "libsml:" prefix and appended newline.

includes a test, test also demonstrates usage.

note that this breaks backwards compatibility (code assuming sml_error exists won't build or run with older versions of the library),
so we should probably bump the version when merging this.

also we should consider if we maybe want a more sophisticated interface.

@r00t- r00t- force-pushed the sml_error_override branch 2 times, most recently from ab2acf3 to 9ad3a4d Compare December 28, 2020 03:07
@r00t- r00t- force-pushed the sml_error_override branch 2 times, most recently from 9e84db4 to b68e635 Compare December 28, 2020 03:15
Copy link

@mbehr1 mbehr1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx! see some comments/suggestions.

strcat(format2, "\n");

vfprintf(stderr, format2, args);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

va_end(args) missing?

va_list args;
va_start(args, format);

char *format2 = malloc(9 + strlen(format) + 1 + 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might check whether format is !=0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems somewhat esoteric, as format will usually be static anyway.
but might throw in a quick `format=format?format:"(null)";


char *format2 = malloc(9 + strlen(format) + 1 + 1);
strcpy(format2, "libsml: ");
strcat(format2, format);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strncpy/strncat? (some compilers/static code checker might sooner or later complain otherwise)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems needless as it's all static strings, i'd leave that to whoever adds that code-checker.

va_list args;
va_start(args, format);

char *format2 = malloc(9 + strlen(format) + 1 + 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: why 9? the terminating zero is already at the end, or? So I'd expect
8 (= strlen("libsml: ") + ... + 1 (\n) + 1 (term. zero)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i miscounted.

@r00t- r00t- changed the title add sml_error function pointer, allow user to intercept error messages WIP: add sml_error function pointer, allow user to intercept error messages Mar 10, 2021
@@ -109,6 +109,16 @@ int sml_buf_optional_is_skipped(sml_buffer *buf);
// Prints arbitrarily byte string to stdout with printf
void hexdump(unsigned char *buffer, size_t buffer_len);

// Allow user to intercept error messages
extern void (*sml_error)(const char *format, ...)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this forces anybody using this to call va_* and vaprintf himself. (see the example in sml_error_default() below)
i wonder if this is any good,
or if we should maybe rather have an intermediate function and pass out the completely formatted string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbehr1:any idea on this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might also pass more details, like a loglevel (warning/error).

va_list args;
va_start(args, format);

char *format2 = malloc(9 + strlen(format) + 1 + 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems somewhat esoteric, as format will usually be static anyway.
but might throw in a quick `format=format?format:"(null)";


char *format2 = malloc(9 + strlen(format) + 1 + 1);
strcpy(format2, "libsml: ");
strcat(format2, format);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems needless as it's all static strings, i'd leave that to whoever adds that code-checker.

va_list args;
va_start(args, format);

char *format2 = malloc(9 + strlen(format) + 1 + 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i miscounted.

@jahir jahir mentioned this pull request Feb 6, 2023
@r00t-
Copy link
Collaborator Author

r00t- commented Feb 9, 2023

@jahir:
you beat me to mentioning this in #109 .
it's sad that this is stalled for 1.5 years, but it got very little feedback after i submitted it.
maybe you can add some?
i'm mostly wondering if the API is sensible, see #93 (comment) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants