Hi Lennart, Please see my respond inline [Vu].
Regards, Vu > -----Original Message----- > From: Lennart Lund [mailto:lennart.l...@ericsson.com] > Sent: Wednesday, August 3, 2016 5:10 PM > To: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>; > mahesh.va...@oracle.com > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [PATCH 1 of 4] log: add one new option -f to saflogger tool > [#1315] > > Hi Vu > > For saflogger tool ACK with minor comments > > See also comments inline [Lennart] > > Thanks > Lennart > > > -----Original Message----- > > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > > Sent: den 1 juli 2016 12:42 > > To: mahesh.va...@oracle.com; Lennart Lund <lennart.l...@ericsson.com> > > Cc: opensaf-devel@lists.sourceforge.net > > Subject: [PATCH 1 of 4] log: add one new option -f to saflogger tool > [#1315] > > > > osaf/tools/saflog/saflogger/Makefile.am | 1 + > > osaf/tools/saflog/saflogger/saf_logger.c | 90 > > ++++++++++++++++++++++++++----- > > 2 files changed, 75 insertions(+), 16 deletions(-) > > > > > > With app stream, saflogger used app stream DN as logFileName. > > With Long DN, the app stream DN could be longer than 255 characters in > > length. > > > > This patch provides one new option -f <filename>. > > This option is only applicable for app stream. > > > > diff --git a/osaf/tools/saflog/saflogger/Makefile.am > > b/osaf/tools/saflog/saflogger/Makefile.am > > --- a/osaf/tools/saflog/saflogger/Makefile.am > > +++ b/osaf/tools/saflog/saflogger/Makefile.am > > @@ -22,6 +22,7 @@ MAINTAINERCLEANFILES = Makefile.in > > bin_PROGRAMS = saflogger > > > > saflogger_CPPFLAGS = \ > > + -DSA_EXTENDED_NAME_SOURCE \ > > $(AM_CPPFLAGS) \ > > -I$(top_srcdir)/osaf/tools/saflog/include > > > > 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 > > @@ -37,8 +37,9 @@ > > #include <poll.h> > > #include <unistd.h> > > #include <limits.h> > > +#include <stdbool.h> > > +#include "osaf_extended_name.h" > > > > -#include <saAis.h> > > #include <saLog.h> > [Lennart] Why removing this include? Many things is this file is used directly > in this file therefore this file should be included here. > If this include is removed it will work anyway since the file is included > indirectly via the osaf_extended_name.h file. This is unclear and creates an > unwanted dependency [Vu] The header "saAis.h" contains core definitions. It must be included in each service API header file (e.g "saLog.h") I think the application no need to include that core header file, include service API header file is enough. > > > > > #include "saf_error.h" > > @@ -83,7 +84,7 @@ static void usage(void) > > printf("\t%s - write log record to log stream\n", progname); > > > > printf("\nSYNOPSIS\n"); > > - printf("\t%s [options] [message ...]\n", progname); > > + printf("\t%s [options] [-f <FILENAME>] [message ...]\n", progname); > > > > printf("\nDESCRIPTION\n"); > > printf("\t%s is a SAF LOG client used to write a log record into a > > specified log stream.\n", progname); > > @@ -95,11 +96,16 @@ static void usage(void) > > printf("\t-n or --notification write to notification stream\n"); > > printf("\t-y or --system write to system stream (default)\n"); > > printf("\t-a NAME or --application=NAME write to application > stream > > NAME (create it if not exist)\n"); > > + printf("\t-f FILENAME write log record to FILENAME\n"); > > printf("\t-s SEV or --severity=SEV use severity SEV, default > > INFO\n"); > > printf("\t\tvalid severity names: emerg, alert, crit, error, warn, > > notice, info\n"); > > + printf("\nNOTES\n"); > > + printf("\t1) -f is only applicable for app stream.\n"); > > + printf("\t1) <FILENAME> length must not be longer than 255 > > characters.\n"); > > > > printf("\nEXAMPLES\n"); > > printf("\tsaflogger -a safLgStrCfg=Test \"Hello world\"\n"); > > + printf("\tsaflogger -a safLgStrCfg=Test -f testLogFile \"Hello > > world\"\n"); > > printf("\tsaflogger -s crit \"I am going down\"\n\n"); > > } > > > > @@ -230,7 +236,7 @@ static SaLogSeverityT get_severity(char > > int main(int argc, char *argv[]) > > { > > int c; > > - SaNameT logStreamName = {.length = 0 }; > > + SaNameT logStreamName; > > SaLogFileCreateAttributesT_2 *logFileCreateAttributes = NULL; > > SaLogFileCreateAttributesT_2 appLogFileCreateAttributes; > > SaLogStreamOpenFlagsT logStreamOpenFlags = 0; > > @@ -253,6 +259,7 @@ int main(int argc, char *argv[]) > > SaLogStreamHandleT logStreamHandle; > > SaSelectionObjectT selectionObject; > > unsigned int wait_time; > > + bool is_appstream = false, f_opt = false; > > > > srandom(getpid()); > > > > @@ -261,11 +268,23 @@ int main(int argc, char *argv[]) > > exit(EXIT_FAILURE); > > } > > > > - sprintf((char *)logSvcUsrName.value, "%s.%u@%s", "saflogger", > > getpid(), hostname); > > - logSvcUsrName.length = strlen((char *)logSvcUsrName.value); > > + if (setenv("SA_ENABLE_EXTENDED_NAMES", "1", 1) != 0) { > > + fprintf(stderr, "Failed to enable Extended SaNameT"); > > + exit(EXIT_FAILURE); > > + } > > + > > + /** > > + * osaf_extended_name_init() is added in case osaf_extended_* > > APIs > > + * are used before saLogInitialize(). > > + */ > > + osaf_extended_name_init(); > > + > > + char svcUserName[kOsafMaxDnLength]; > > + snprintf(svcUserName, sizeof(svcUserName), "%s.%u@%s", > > "saflogger", getpid(), hostname); > > + saAisNameLend(svcUserName, &logSvcUsrName); > > > > /* Setup default values */ > > - strcpy((char *)logStreamName.value, SA_LOG_STREAM_SYSTEM); > > /* system stream is default */ > > + saAisNameLend(SA_LOG_STREAM_SYSTEM, &logStreamName); > > logRecord.logTimeStamp = SA_TIME_UNKNOWN; /* LOG > > service should supply timestamp */ > > logRecord.logHdrType = SA_LOG_GENERIC_HEADER; > > logRecord.logHeader.genericHdr.notificationClassId = NULL; > > @@ -280,32 +299,58 @@ int main(int argc, char *argv[]) > > appLogFileCreateAttributes.maxFilesRotated = > > DEFAULT_MAX_FILES_ROTATED; > > /* Use built-in log file format in log server for app stream */ > > appLogFileCreateAttributes.logFileFmt = NULL; > > + appLogFileCreateAttributes.logFileName = NULL; > > > > while (1) { > > - c = getopt_long(argc, argv, "?hlnya:s:", long_options, NULL); > > + c = getopt_long(argc, argv, "?hlnya:s:f:", long_options, > > NULL); > > if (c == -1) { > > break; > > } > > switch (c) { > > case 'l': > > - strcpy((char *)logStreamName.value, > > SA_LOG_STREAM_ALARM); > > + saAisNameLend(SA_LOG_STREAM_ALARM, > > &logStreamName); > > logRecord.logHdrType = SA_LOG_NTF_HEADER; > > break; > > case 'n': > > - strcpy((char *)logStreamName.value, > > SA_LOG_STREAM_NOTIFICATION); > > + saAisNameLend(SA_LOG_STREAM_NOTIFICATION, > > &logStreamName); > > logRecord.logHdrType = SA_LOG_NTF_HEADER; > > break; > > case 'y': > > - strcpy((char *)logStreamName.value, > > SA_LOG_STREAM_SYSTEM); > > + saAisNameLend(SA_LOG_STREAM_SYSTEM, > > &logStreamName); > > break; > > case 'a': > > + logFileCreateAttributes = > > &appLogFileCreateAttributes; > > + logStreamOpenFlags = SA_LOG_STREAM_CREATE; > > + > > + char tmpDn[kOsafMaxDnLength + 8 + 1] = {0}; > > if (strstr(optarg, "safLgStr")) > > - strcpy((char *)logStreamName.value, > > optarg); > > + strncpy(tmpDn, optarg, sizeof(tmpDn) - 1); > > else > > - sprintf((char *)logStreamName.value, > > "safLgStr=%s", optarg); > > - logFileCreateAttributes = > > &appLogFileCreateAttributes; > > + snprintf(tmpDn, sizeof(tmpDn), > > "safLgStr=%s", optarg); > > + > > + if (strlen(tmpDn) > kOsafMaxDnLength) { > > + fprintf(stderr, "Application stream DN is so > > long (%lu). Max: %d \n", > > + strlen(optarg), kOsafMaxDnLength); > > + fprintf(stderr, "Shut down app. \n"); > > + exit(EXIT_FAILURE); > > + } > > + saAisNameLend(tmpDn, &logStreamName); > > + is_appstream = true; > > + if (f_opt == false) > > + appLogFileCreateAttributes.logFileName = > > strdup(optarg); > > + break; > > + case 'f': > > + if (f_opt == true) { > > + fprintf(stderr, "More than one option -f are > > given.\n"); > > + fprintf(stderr, "Try saflogger -h for more > > information.\n"); > > + exit(EXIT_FAILURE); > > + > > + } > > + if (appLogFileCreateAttributes.logFileName != NULL) > > + > > free(appLogFileCreateAttributes.logFileName); > > + > > appLogFileCreateAttributes.logFileName = > > strdup(optarg); > > - logStreamOpenFlags = SA_LOG_STREAM_CREATE; > > + f_opt = true; > > break; > > case 's': > > logRecord.logHeader.genericHdr.logSeverity = > > get_severity(optarg); > > @@ -322,6 +367,21 @@ int main(int argc, char *argv[]) > > } > > } > > > > + if (f_opt == true && is_appstream == false) { > > + fprintf(stderr, "Note: -f is only applicaple for app stream.\n"); > > + fprintf(stderr, "Try saflogger -h for more information.\n"); > > + exit(EXIT_FAILURE); > > + } > > + > > + if (appLogFileCreateAttributes.logFileName != NULL && > > is_appstream == true) { > > + size_t len = 0; > > + if ((len = strlen(appLogFileCreateAttributes.logFileName)) > > > 255) { > > + fprintf(stderr, "FILENAME is too long (%zu) (max: 255 > > characters).\n", len); > > + fprintf(stderr, "Try saflogger -h for more > > information.\n"); > > + exit(EXIT_FAILURE); > > + } > > + } > > + > > if (optind >= argc) { > > /* No body of log record */ > > } > > @@ -351,8 +411,6 @@ int main(int argc, char *argv[]) > > 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; ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
