zowelog zss format handler #126
Conversation
Signed-off-by: sakeerthy <[email protected]>
Signed-off-by: sakeerthy <[email protected]>
Signed-off-by: sakeerthy <[email protected]>
Signed-off-by: sakeerthy <[email protected]>
Signed-off-by: sakeerthy <[email protected]>
| #endif | ||
|
|
||
| zowelog(NULL, LOG_COMP_ID_CMS, ZOWE_LOG_INFO, strcat(formatStringPrefix, |
There was a problem hiding this comment.
Do you strcat into a string literal? Have you tested this code? I would expect an S0C4.
There was a problem hiding this comment.
Agreed! it's a buffer overflow into the literal pool! Unfortunately, it may not ABEND, it will just clobber the literal pool. strcat is a code smell and should never be used.
| #define LOG_DEST_DEV_NULL 0x008F0000 | ||
| #define LOG_DEST_PRINTF_STDOUT 0x008F0001 | ||
| #define LOG_DEST_PRINTF_STDERR 0x008F0002 | ||
|
|
||
| struct LoggingContext_tag; | ||
|
|
||
| typedef void (*LogHandler)(struct LoggingContext_tag *context, | ||
| LoggingComponent *component, | ||
| LoggingComponent *component, |
There was a problem hiding this comment.
This breaks all the code that uses logging, we shouldn't make such changes in minor releases. If you kept compatibility you wouldn't have to change the cross memory code. Imagine that someone uses this API, do they now need to change their code?
What about binaries? You will break existing plugins because you'll call theirs handler assuming it's something else.
Please reconsider this design. You may want to introduce another handler type, allow new code to set it and check at runtime whether you need to caller the old handler type or the new one. That way you don't have to change so much code and break old existing code.
Same applies to the dump function.
| #define LOG_DEST_DEV_NULL 0x008F0000 | ||
| #define LOG_DEST_PRINTF_STDOUT 0x008F0001 | ||
| #define LOG_DEST_PRINTF_STDERR 0x008F0002 | ||
|
|
||
| struct LoggingContext_tag; | ||
|
|
||
| typedef void (*LogHandler)(struct LoggingContext_tag *context, | ||
| LoggingComponent *component, | ||
| LoggingComponent *component, | ||
| char* path, int line, |
There was a problem hiding this comment.
Do not mix tabs and spaces. This applies to all the changes in this PR.
| @@ -332,8 +360,8 @@ int logGetLevel(LoggingContext *context, uint64 compID); | |||
| extern int logSetExternalContext(LoggingContext *context); | |||
| extern LoggingContext *logGetExternalContext(); | |||
|
|
|||
| void printStdout(LoggingContext *context, LoggingComponent *component, void *data, char *formatString, va_list argList); | |||
| void printStderr(LoggingContext *context, LoggingComponent *component, void *data, char *formatString, va_list argList); | |||
| void printStdout(LoggingContext *context, LoggingComponent *component, char* path, int line, int level, uint64 compID, void *data, char *formatString, va_list argList); | |||
There was a problem hiding this comment.
Do not change existing APIs.
| #define LOGCHECK(context,component,level) \ | ||
| ((component > MAX_LOGGING_COMPONENTS) ? \ | ||
| (context->applicationComponents[component].level >= level) : \ | ||
| (context->coreComponents[component].level >= level) ) | ||
|
|
||
|
|
||
| #define zowelog(context, compID, level, formatString, ...) \ | ||
| _zowelog(context, compID, __FILE__, __LINE__, level, formatString, ##__VA_ARGS__); |
There was a problem hiding this comment.
By introducing a macro we will break existing plugins because they assume that zowelog has fewer args. It may be better to have zowelog2 to keep the backward compatibility.
Dependent on zowe/zss#160
Signed-off-by: sakeerthy [email protected]