Thanks Lennart for your info. I will put that header back before pushing. Regards, Vu
> -----Original Message----- > From: Lennart Lund [mailto:lennart.l...@ericsson.com] > Sent: Thursday, August 4, 2016 2: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 > > [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. > > It is correct as you say that saAis.h has to be included in almost every file but > as a general rule I still think it is better to directly include all header files that > contains anything that is used directly and to not include files that does not > contain anything that is used directly. Also header files must contain all > includes used directly, there shall not be any dependency on that some other > header file has been included. > A good thing to do and this will ensure that a header file is indeed > independent is to always include the .h file as the first include in the > corresponding .c file when applicable. > With this said my ACK is still valid whatever your decision is in this matter > > Thanks > Lennart > > > -----Original Message----- > > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > > Sent: den 4 augusti 2016 07:07 > > To: Lennart Lund <lennart.l...@ericsson.com>; 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 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
