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__ ?
>
> -- 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 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");
>
> /* 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.
[...]
> /* 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
Which could be pretty confusing when looking through the logs some time
later (where'd the ERROR logs go?). I think levels are better.
> //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?
> 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.
> //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.
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
