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
