Hi Giang, I could apply the attached patch without problem.
Ack, with a minor comment. In the option switch there is a "case '?':", this is meaningless unless the option string also contains this character e.g. "?hlnya:s:" If Mathi has no more comments I can fix this and push this patch. Thanks Lennart -----Original Message----- From: giang do [mailto:[email protected]] Sent: den 3 september 2015 09:27 To: Mathivanan Naickan Palanivelu; Vu Nguyen M; Lennart Lund Cc: [email protected] Subject: Re: [PATCH 1 of 1] log: saflogger does not check invalid options [#1367] Hi Mathi, I also tried to applied my patch that sent to you but I can not. I don't know why. Please try my patch in attachment again. About freeing logBuf: Because string that pointed by logBug will be used later in write_log_record(logHandle, logStreamHandle, selectionObject, &logRecord), we can not free it immediately. Instead we should free logRecord.logBuffer->logBuf. But, because not freeing this pointer do not cause serious memory leak, I think we can ignore it. I've try to run valgrind and the the result just say that this is "still reachable". Base on this discussion I think we can ignore freeing that pointer: http://stackoverflow.com/questions/3840582/still-reachable-leak-detected-by-valgrind valgrind --show-reachable=yes --leak-check=full --track-origins=yes /usr/local/bin/saflogger abc ==750== Thread 1: ==750== 32 bytes in 1 blocks are still reachable in loss record 1 of 2 ==750== at 0x4C2C494: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==750== by 0x589768F: _dlerror_run (dlerror.c:141) ==750== by 0x58970C0: dlopen@@GLIBC_2.2.5 (dlopen.c:87) ==750== by 0x5066FB4: ncs_leap_startup (ncs_main_pub.c:254) ==750== by 0x5067306: ncs_core_agents_startup (ncs_main_pub.c:337) ==750== by 0x5067448: ncs_agents_startup (ncs_main_pub.c:180) ==750== by 0x52C9564: lga_startup (lga_util.c:282) ==750== by 0x52C799D: saLogInitialize (lga_api.c:131) ==750== by 0x401495: main (saf_logger.c:368) ==750== ==750== 67 bytes in 1 blocks are still reachable in loss record 2 of 2 ==750== at 0x4C2A2DB: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==750== by 0x4016A6: main (saf_logger.c:333) ==750== ==750== LEAK SUMMARY: ==750== definitely lost: 0 bytes in 0 blocks ==750== indirectly lost: 0 bytes in 0 blocks ==750== possibly lost: 0 bytes in 0 blocks ==750== still reachable: 99 bytes in 2 blocks ==750== suppressed: 0 bytes in 0 blocks Best regards, Giang Do On 01/09/2015 12:34, Mathivanan Naickan Palanivelu wrote: > Hi Giang, > > Iam unable to apply the patch on the latest staging. > However, the changes look straight forward. > Should we also free logBuf? > > Thanks, > Mathi. > >> -----Original Message----- >> From: giang [mailto:[email protected]] >> Sent: Tuesday, August 25, 2015 11:24 AM >> To: Mathivanan Naickan Palanivelu; [email protected]; >> [email protected] >> Cc: [email protected] >> Subject: [PATCH 1 of 1] log: saflogger does not check invalid options >> [#1367] >> >> osaf/tools/saflog/saflogger/saf_logger.c | 49 >> ++++++++++++++++++++++--------- >> 1 files changed, 35 insertions(+), 14 deletions(-) >> >> >> If there are invalid non-options, show them, recommend use --help and >> then exit. >> >> diff --git a/osaf/tools/saflog/saflogger/saf_logger.c >> b/osaf/tools/saflog/saflogger/saf_logger.c >> --- a/osaf/tools/saflog/saflogger/saf_logger.c >> +++ b/osaf/tools/saflog/saflogger/saf_logger.c >> @@ -311,14 +311,48 @@ int main(int argc, char *argv[]) >> logRecord.logHeader.genericHdr.logSeverity = >> get_severity(optarg); >> break; >> case 'h': >> + usage(); >> + exit(EXIT_SUCCESS); >> + break; >> case '?': >> default: >> - usage(); >> + fprintf(stderr, "Try saflogger -h for more >> information.\n"); >> exit(EXIT_FAILURE); >> break; >> } >> } >> >> + if (optind >= argc) { >> + /* No body of log record */ >> + } >> + else if (optind == argc - 1) { >> + /* Create body of log record */ >> + int sz; >> + char *logBuf = NULL; >> + sz = strlen(argv[optind]); >> + logBuf = malloc(sz + 64); /* add space for index/id in >> periodic writes */ >> + strcpy(logBuf, argv[optind]); >> + logBuffer.logBufSize = sz; >> + logBuffer.logBuf = (SaUint8T *)logBuf; >> + logRecord.logBuffer = &logBuffer; >> + } >> + else { >> + fprintf(stderr, "Invalid argument.\n"); >> + fprintf(stderr, "Enclose message in quotation marks \"\" e.g. >> \""); >> + while (optind < argc) >> + { >> + fprintf(stderr, "%s", argv[optind++]); >> + if (optind < argc) >> + fprintf(stderr, " "); >> + else >> + fprintf(stderr, "\"\n"); >> + } >> + fprintf(stderr, "Try saflogger -h for more information.\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + logStreamName.length = strlen((char *)logStreamName.value); >> + >> if (logRecord.logHdrType == SA_LOG_NTF_HEADER) { >> /* Setup some valid values */ >> logRecord.logHeader.ntfHdr.notificationId = >> SA_NTF_IDENTIFIER_UNUSED; @@ -329,19 +363,6 @@ int main(int argc, >> char >> *argv[]) >> logRecord.logHeader.ntfHdr.eventTime = get_current_SaTime(); >> } >> >> - logStreamName.length = strlen((char *)logStreamName.value); >> - >> - /* Create body of log record (if any) */ >> - if (optind < argc) { >> - int sz; >> - char *logBuf = NULL; >> - sz = strlen(argv[optind]); >> - logBuf = malloc(sz + 64); /* add space for index/id in >> periodic writes */ >> - strcpy(logBuf, argv[optind]); >> - logBuffer.logBufSize = sz; >> - logBuffer.logBuf = (SaUint8T *)logBuf; >> - logRecord.logBuffer = &logBuffer; >> - } >> >> wait_time = 0; >> error = saLogInitialize(&logHandle, &logCallbacks, &logVersion); ------------------------------------------------------------------------------ Monitor Your Dynamic Infrastructure at Any Scale With Datadog! Get real-time metrics from all of your servers, apps and tools in one place. SourceForge users - Click here to start your Free Trial of Datadog now! http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140 _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
