Thanks for the feedback! You didn't miss any functionality, there was too much functionality!?!
On 18 August 2014 17:16, Stuart Haslam <[email protected]> wrote: > On Fri, Aug 15, 2014 at 04:17:22PM +0100, Ola Liljedahl wrote: > > (This document/code contribution attached is provided under the terms of > agreement LES-LTM-21309) > > > > I contribute a proposal for a log API for ODP to stimulate further > discussion. > > > > Some comments: > > Header file missing compile-time selection of log/severity levels, left > as an exercise to the mailing list. > > Implementation is not thread safe but that should be easy to fix, > thread-unsafe operations are not performance critical so some coarse > locking should be fine. > > Example usage in log_example.c. This is really the important part. > > > > Build with > > gcc -std=c99 -Wall odp_log.c log_example.c -o log_example > > > > There might be one thing that is GCC-specific, see if you can find it. > > Is there a prize? > > ##__VAR_ARGS__ ? > Half-correct so no price for you. It's the combination of "##__VAR_ARGS" and the preceding "," that swallows the "," when __VAR_ARGS__ is empty. A GNU extension. Using other compilers the programmer might have to specify a dummy argument after the format string. > > > > -- Ola > > > > > /* Copyright (c) 2013, Linaro Limited > > * All rights reserved. > > * > > * SPDX-License-Identifier: BSD-3-Clause > > */ > > > > > > /** > > * @file > > * > > * Simplistic ODP logging, lacks mutual exclusion for multithreading > > */ > > > > #include <limits.h> > > #include <stdint.h> > > #include <stdarg.h> > > #include <stdio.h> > > #include <stdlib.h> > > #include <string.h> > > #include <strings.h> > > #include "odp_log.h" > > > > static odp_log_source_t *first = NULL; > > static odp_log_callback_t callback = NULL; > > static ODP_LOG_SOURCE(odp_log); > > > > void odp_log_init(void) > > { > > ODP_LOG_REGISTER(odp_log); > > odp_log_set_mask("odp_log", ODP_LOG_MASK_ALL); > > } > > I find the semantics unusual, in particular the scope of odp_log and the > fact that it's initialised unquoted and then used quoted. > The client API uses the source name unquoted, all client calls are done using macros. The management API (which may be called from anywhere) specifies the log sources using quoted strings. Possibly this macro thing puts too many restrictions on the scope of the log source variable. > The code above is not so bad due to the global scope of odp_log, but in > the example with local scope it's not clear whether you're registering > with a global handler or initialising a local variable. > > To me something like this would be more natural; > > static odp_log_source_t *odp_log; > odp_log = odp_log_register("odp_log"); > ODP_LOG_INFORM(odp_log, "bleh bleh\n"); > This could work and is probably better since the source variable can be anywhere. I will modify and see how it works out. > > > > > /* Register a log source with the specified name */ > > int odp_log_register_source(odp_log_source_t *src, const char *name) > > { > > odp_log_source_t **pptr = &first; > > while (*pptr != NULL) > > { > > /* Is new element smaller than existing? */ > > if (strcmp(name, (*pptr)->name) < 0) > > { > > insert: > > src->next = *pptr; > > src->name = name; > > src->mask = ODP_LOG_SEV_FATAL | ODP_LOG_SEV_ERROR | > > ODP_LOG_SEV_INFORM; > > *pptr = src; > > return 0; > > } > > pptr = &(*pptr)->next; > > } > > /* Traversed whole list, insert at end */ > > goto insert; > > } > > That's a weird use of goto, it would be clearer to move stuff within the > strcmp block after the loop, reverse the logic of the strcmp and break. > You are right. I just wrote this code and made it work and didn't think too hard on the best way of expressing the functionality. I didn't propose this as the proper implementation, I just needed something that worked so I could test the API. > > [...] > > > /* Copyright (c) 2013, Linaro Limited > > * All rights reserved. > > * > > * SPDX-License-Identifier: BSD-3-Clause > > */ > > > > > > /** > > * @file > > * > > * ODP logging > > */ > > > > #include <stdio.h> > > #include <string.h> > > #include <strings.h> > > #include "odp_log.h" > > > > static const char *severity[] = > > { > > "fatal", "error", "warning", "informational", "debug" > > }; > > > > void my_vprintf(const char *name, odp_log_mask_t sev, const char *fmt, > va_list ap) > > { > > printf("%s/%s: ", name, severity[ffs(sev) - 1]); > > vprintf(fmt, ap); > > } > > > > int main() > > { > > odp_log_init(); > > > > ODP_LOG_SOURCE(odp_queue); > > //Register log source and set default log mask > > ODP_LOG_REGISTER(odp_queue); > > //Log a message with severity error > > ODP_LOG_ERROR(odp_queue, "Failed to create queue, param=%u\n", 242); > > > > //Set log mask for a source to log everything > > (void)odp_log_set_mask("odp_queue", ODP_LOG_MASK_ALL); > > Using a mask makes this valid; > > ODP_LOG_SEV_FATAL & ODP_LOG_SEV_DEBUG > Any programmer knows that you express and expressions using or! "I want A and B" => A | B. > > Which could be pretty confusing when looking through the logs some time > later (where'd the ERROR logs go?). I think levels are better. > I started with levels but then realized that the different types didn't create a clear hierarchy. Where does INFORM go into a level-based hierarchy? If we can agree on a hierarchy of fatal, error, warning, debug and inform "levels", I am for this, I agree it seems more natural. But it is also more limiting. > > //Install a custom vprintf function to handle log messages > > odp_log_set_callback(my_vprintf); > > ODP_LOG_INFORM(odp_queue, "Message printed using user-defined > call-back\n"); > > > > //Add some source out-of-alphabetical order > > ODP_LOG_SOURCE(b); ODP_LOG_REGISTER(b); > > ODP_LOG_SOURCE(a); ODP_LOG_REGISTER(a); > > ODP_LOG_SOURCE(c); ODP_LOG_REGISTER(c); > > //List all registered log sources > > char name[ODP_NAME_MAX + 1]; > > odp_log_mask_t mask; > > strcpy(name, ""); //Empty name is before all other names > > I don't understand the above comment, does that mean "" is a valid log > source? can I print to it? > No empty string is not a valid log source. It might not be checked for but you will struggle to define it using the current set of client-side macros. The empty string is used here as a cursor into the list of log sources (alphabetically sorted), before all elements. Keep the state in the client, not in the server. > > > while (odp_log_next_source(name, &mask) == 0) > > { > > printf("Log source %s, log mask 0x%x\n", name, mask); > > } > > > > I can't think why you'd want to iterate through the log sources. > Try harder. See it from the management perspective. Maybe there is a GUI where you can filter and list the log messages, list all log sources and change the log masks. You should get a demo of Enea Element, the "web" interface (browser-based GUI) is really advanced. > > > //We are done, unregister source > > ODP_LOG_UNREGISTER(odp_queue); > > //A redundant unregister, just for fun > > ODP_LOG_UNREGISTER(odp_queue); > > > > What about ODP_LOG_UNSOURCE()?.. just kidding, I know there isn't one as > it just goes out of scope, but it does make the function look unbalanced. > Source normally live forever but I wanted a mean to explicitly remove one. > > ODP_LOG_SOURCE(odp_queue); > ODP_LOG_REGISTER(odp_queue); > .. > ODP_LOG_UNREGISTER(odp_queue); > > > return 0; > > } > > -- > Stuart. > >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
