Hi Vu Ack See my comments inline
Thanks Lennart > -----Original Message----- > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > Sent: den 13 oktober 2015 10:37 > To: Lennart Lund; mathi.naic...@oracle.com; Giang Do T > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [PATCH 1 of 1] log: fix crashed with > saLogStreamFixedLogRecordSize=1 [#1466] > > Hi Lennart, > > Thanks for your comments. Please see my response inline. > > Regards, > Vu > > >-----Original Message----- > >From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >Sent: Tuesday, October 13, 2015 3:02 PM > >To: Vu Nguyen M; mathi.naic...@oracle.com; Giang Do T > >Cc: opensaf-devel@lists.sourceforge.net > >Subject: RE: [PATCH 1 of 1] log: fix crashed with > >saLogStreamFixedLogRecordSize=1 [#1466] > > > >Hi Vu, > > > >See my comments inline > > > >Thanks > >Lennart > > > >> -----Original Message----- > >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > >> Sent: den 7 oktober 2015 09:28 > >> To: mathi.naic...@oracle.com; Lennart Lund; Giang Do T > >> Cc: opensaf-devel@lists.sourceforge.net > >> Subject: [PATCH 1 of 1] log: fix crashed with > >> saLogStreamFixedLogRecordSize=1 [#1466] > >> > >> osaf/libs/agents/saf/lga/lga_api.c | 8 ++ > >> osaf/libs/saf/include/saLog.h | 4 + > >> osaf/services/saf/logsv/lgs/lgs_config.c | 3 +- > >> osaf/services/saf/logsv/lgs/lgs_evt.c | 9 ++ > >> osaf/services/saf/logsv/lgs/lgs_imm.c | 7 ++ > >> osaf/tools/saflog/saflogger/saf_logger.c | 2 +- > >> tests/logsv/logtest.h | 2 +- > >> tests/logsv/saflogtest.c | 2 +- > >> tests/logsv/tet_LogOiOps.c | 24 ++++++- > >> tests/logsv/tet_saLogStreamOpen_2.c | 104 > >> +++++++++++++++++++++++++++++++ > >> 10 files changed, 158 insertions(+), 7 deletions(-) > >> > >> > >> The mininum value should be 150 if not 0 as the log record > >> with no information is meaningless. > >> > >> Also validate if the value is in range for runtime stream > >> which created via LOG API. > >> > >> diff --git a/osaf/libs/agents/saf/lga/lga_api.c > >> b/osaf/libs/agents/saf/lga/lga_api.c > >> --- a/osaf/libs/agents/saf/lga/lga_api.c > >> +++ b/osaf/libs/agents/saf/lga/lga_api.c > >> @@ -538,6 +538,14 @@ static SaAisErrorT validate_open_params( > >> return SA_AIS_ERR_INVALID_PARAM; > >> } > >> > >> + /* Verify that the > >> fixedLogRecordSize is in valid range */ > >> + if ((logFileCreateAttributes- > >> >maxLogRecordSize != 0) && > >> + > >> ((logFileCreateAttributes->maxLogRecordSize < > >> SA_LOG_MIN_RECORD_SIZE) || > >> + > >> (logFileCreateAttributes->maxLogRecordSize > > >> SA_LOG_MAX_RECORD_SIZE))) { > >> + > >> TRACE("maxLogRecordSize is invalid"); > >> + return > >> SA_AIS_ERR_INVALID_PARAM; > >> + } > >> + > >> /* Validate maxFilesRotated just > >> in case of SA_LOG_FILE_FULL_ACTION_ROTATE type */ > >> if ((logFileCreateAttributes- > >> >logFileFullAction == SA_LOG_FILE_FULL_ACTION_ROTATE) && > >> > >> ((logFileCreateAttributes->maxFilesRotated < 1) || > >> diff --git a/osaf/libs/saf/include/saLog.h > b/osaf/libs/saf/include/saLog.h > >> --- a/osaf/libs/saf/include/saLog.h > >> +++ b/osaf/libs/saf/include/saLog.h > >> @@ -54,6 +54,10 @@ extern "C" { > >> #define SA_LOG_STREAM_NOTIFICATION > >> "safLgStrCfg=saLogNotification,safApp=safLogService" > >> #define SA_LOG_STREAM_ALARM > >> "safLgStrCfg=saLogAlarm,safApp=safLogService" > >> > >> +/* Valid range of fixedLogRecordSize */ > >> +#define SA_LOG_MAX_RECORD_SIZE 65535 > >> +#define SA_LOG_MIN_RECORD_SIZE 150 > >> + > >> /* The Log severity level FLAGS */ > >> #define SA_LOG_SEV_EMERGENCY 0 > >> #define SA_LOG_SEV_ALERT 1 > >> diff --git a/osaf/services/saf/logsv/lgs/lgs_config.c > >> b/osaf/services/saf/logsv/lgs/lgs_config.c > >> --- a/osaf/services/saf/logsv/lgs/lgs_config.c > >> +++ b/osaf/services/saf/logsv/lgs/lgs_config.c > >> @@ -551,7 +551,8 @@ int lgs_cfg_verify_log_file_format(const > >> int lgs_cfg_verify_max_logrecsize(uint32_t max_logrecsize_in) > >> { > >> int rc = 0; > >> - if ((max_logrecsize_in > 65535) || (max_logrecsize_in < 150)) { > >> + if ((max_logrecsize_in > SA_LOG_MAX_RECORD_SIZE) || > >> + (max_logrecsize_in < > >> SA_LOG_MIN_RECORD_SIZE)) { > >> LOG_NO("verify_max_logrecsize Fail"); > >> rc = -1; > >> } > > > >[Lennart] A max_logrecsize of 0 is also allowed > >Also we seems to miss a test case verifying that logMaxLogrecsize can be > set to > >0 that would have failed on this error > [Vu] In the current design, ` saLogStreamFixedLogRecordSize` is allowed to > set to zero (0). > In that case, the length of log record is variable and up to max_logrecord > (`logMaxLogrecsize`) in maximum. > > If max_logrecord is allowed to set to zero (0), what happens if setting > `saLogStreamFixedLogRecordSize` to zero(0)? > Please note that, maxLogRecordSize passed to LOG Open API is used for ` > saLogStreamFixedLogRecordSize` attribute value, > therefore the value zero (0) is allowed to set to. > > Above function should not be used to validate the maxLogRecordSize (` > saLogStreamFixedLogRecordSize` ) passing to Open API. > [Lennart] Yes you are right. I have mixed up max logrecsize configuration setting and max logrecsize for a stream! > > > >> diff --git a/osaf/services/saf/logsv/lgs/lgs_evt.c > >> b/osaf/services/saf/logsv/lgs/lgs_evt.c > >> --- a/osaf/services/saf/logsv/lgs/lgs_evt.c > >> +++ b/osaf/services/saf/logsv/lgs/lgs_evt.c > >> @@ -838,6 +838,15 @@ static SaAisErrorT create_new_app_stream > >> goto done; > >> } > >> > >> + /* Verify that the fixedLogRecordSize is in valid range */ > >> + if ((open_sync_param->maxLogRecordSize != 0) && > >> + ((open_sync_param->maxLogRecordSize < > >> SA_LOG_MIN_RECORD_SIZE) || > >> + (open_sync_param->maxLogRecordSize > > >> SA_LOG_MAX_RECORD_SIZE))) { > >> + TRACE("maxLogRecordSize is invalid"); > >> + rc = SA_AIS_ERR_INVALID_PARAM; > >> + goto done; > >> + } > >> + > >> stream = log_stream_new(&open_sync_param->lstr_name, > >> open_sync_param- > >> >logFileName, > >> > >[Lennart] Validation function lgs_cfg_verify_max_logrecsize() shall be used > >here. > >This is code duplication and also the validation rule should be defined in > one > >place. > [Vu] > lgs_cfg_verify_max_logrecsize() is used to validate ` logMaxLogrecsize` > attribute. [Lennart] Yes my mistake. Same as above > > Seems I forgot one condition check here to make sure `maxLogRecordSize` is > not larger than `logMaxLogrecsize`. > Something likes: > > SaUint32T logMaxLogrecsize_conf = *(SaUint32T *) > lgs_cfg_get(LGS_IMM_LOG_MAX_LOGRECSIZE); > > /* Verify that the fixedLogRecordSize is in valid range */ > if ((open_sync_param->maxLogRecordSize != 0) && > ((open_sync_param->maxLogRecordSize < > SA_LOG_MIN_RECORD_SIZE) || > (open_sync_param->maxLogRecordSize > > logMaxLogrecsize_conf))) { > TRACE("maxLogRecordSize is invalid"); > rc = SA_AIS_ERR_INVALID_PARAM; > goto done; > } > > stream = log_stream_new(...) > > > > > open_sync_param- > >> >logFilePathName, > >> diff --git a/osaf/services/saf/logsv/lgs/lgs_imm.c > >> b/osaf/services/saf/logsv/lgs/lgs_imm.c > >> --- a/osaf/services/saf/logsv/lgs/lgs_imm.c > >> +++ b/osaf/services/saf/logsv/lgs/lgs_imm.c > >> @@ -1195,6 +1195,13 @@ static bool chk_max_filesize_recordsize_ > >> /* streamFixedLogRecordSize can > >> be 0. Means variable record size */ > >> > >> TRACE("streamFixedLogRecordSize = 0"); > >> rc = true; > >> + } else if (i_streamFixedLogRecordSize < > >> SA_LOG_MIN_RECORD_SIZE) { > >> + /* streamFixedLogRecordSize > >> must be larger than 150 */ > >> + report_oi_error(immOiHandle, > >> ccbId, > >> + > >> "streamFixedLogRecordSize out of range"); > >> + > >> TRACE("i_streamFixedLogRecordSize (%d) < %d", > >> + > >> i_streamFixedLogRecordSize, SA_LOG_MIN_RECORD_SIZE); > >> + rc = false; > >> } else if (i_streamFixedLogRecordSize >= > >> i_streamMaxLogFileSize) { > >> /* streamFixedLogRecordSize > >> must be less than streamMaxLogFileSize */ > >> report_oi_error(immOiHandle, > >> ccbId, > >> 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 > >> @@ -44,7 +44,7 @@ > >> #include "saf_error.h" > >> > >> #define DEFAULT_FORMAT_EXPRESSION "@Cr @Ch:@Cn:@Cs > >> @Cm/@Cd/@CY @Sv @Sl \"@Cb\"" > >> -#define DEFAULT_APP_LOG_REC_SIZE 128 > >> +#define DEFAULT_APP_LOG_REC_SIZE 150 > >> #define DEFAULT_APP_LOG_FILE_SIZE 1024 > >> #define VENDOR_ID 193 > >> #define DEFAULT_MAX_FILES_ROTATED 4 > >> diff --git a/tests/logsv/logtest.h b/tests/logsv/logtest.h > >> --- a/tests/logsv/logtest.h > >> +++ b/tests/logsv/logtest.h > >> @@ -31,7 +31,7 @@ > >> #define SA_LOG_STREAM_APPLICATION1 "safLgStr=saLogApplication1" > >> #define SA_LOG_STREAM_APPLICATION2 "safLgStr=saLogApplication2" > >> #define SA_LOG_STREAM_APPLICATION3 "safLgStr=saLogApplication3" > >> -#define DEFAULT_APP_LOG_REC_SIZE 128 > >> +#define DEFAULT_APP_LOG_REC_SIZE 150 > >> #define DEFAULT_APP_LOG_FILE_SIZE 1024 * 1024 > >> #define DEFAULT_FORMAT_EXPRESSION "@Cr @Ch:@Cn:@Cs > >> @Cm/@Cd/@CY @Sv @Sl \"@Cb\"" > >> #define DEFAULT_ALM_LOG_REC_SIZE 1024 > >> diff --git a/tests/logsv/saflogtest.c b/tests/logsv/saflogtest.c > >> --- a/tests/logsv/saflogtest.c > >> +++ b/tests/logsv/saflogtest.c > >> @@ -46,7 +46,7 @@ > >> #include <saLog.h> > >> > >> #define DEFAULT_FORMAT_EXPRESSION "@Cr @Ch:@Cn:@Cs > >> @Cm/@Cd/@CY @Sv @Sl \"@Cb\"" > >> -#define DEFAULT_APP_LOG_REC_SIZE 128 > >> +#define DEFAULT_APP_LOG_REC_SIZE 150 > >> #define DEFAULT_APP_LOG_FILE_SIZE 1024 * 1024 > >> #define VENDOR_ID 193 > >> #define DEFAULT_MAX_FILES_ROTATED 4 > >> diff --git a/tests/logsv/tet_LogOiOps.c b/tests/logsv/tet_LogOiOps.c > >> --- a/tests/logsv/tet_LogOiOps.c > >> +++ b/tests/logsv/tet_LogOiOps.c > >> @@ -595,14 +595,14 @@ void saLogOi_29() > >> } > >> > >> /** > >> - * CCB Object Modify, saLogStreamFixedLogRecordSize=80, strC > >> + * CCB Object Modify, saLogStreamFixedLogRecordSize=150, strC > >> */ > >> void saLogOi_30() > >> { > >> int rc; > >> char command[256]; > >> > >> - sprintf(command, "immcfg -a saLogStreamFixedLogRecordSize=80 > >> safLgStrCfg=strC,safApp=safLogService"); > >> + sprintf(command, "immcfg -a saLogStreamFixedLogRecordSize=150 > >> safLgStrCfg=strC,safApp=safLogService"); > >> rc = system(command); > >> rc_validate(WEXITSTATUS(rc), 0); > >> } > >> @@ -3301,6 +3301,23 @@ void verAdminOpOnConfClass(void) > >> rc = system(command); > >> } > >> > >> +/* Add test case to verify #1466 */ > >> + > >> +/** > >> + * saLogStreamFixedLogRecordSize is in range [150 - MAX_RECSIZE] if not > 0. > >> + * Verify that setting to value less than 150 is not allowed. > >> + */ > >> +void verFixLogRec_Min(void) > >> +{ > >> + int rc; > >> + char command[256]; > >> + > >> + sprintf(command, "immcfg -a saLogStreamFixedLogRecordSize=149 %s > 2> > >> /dev/null", > >> + SA_LOG_STREAM_ALARM); > >> + rc = system(command); > >> + rc_validate(WEXITSTATUS(rc), 1); > >> +} > >> + > >> __attribute__ ((constructor)) static void > saOiOperations_constructor(void) > >> { > >> /* Stream objects */ > >> @@ -3337,7 +3354,7 @@ void verAdminOpOnConfClass(void) > >> test_case_add(4, saLogOi_25, "CCB Object Create, strC"); > >> test_case_add(4, saLogOi_28, "CCB Object Modify, > >> saLogStreamMaxFilesRotated=1, strA"); > >> test_case_add(4, saLogOi_29, "CCB Object Modify, > >> saLogStreamMaxLogFileSize=0, strB, ERR not supported"); > >> - test_case_add(4, saLogOi_30, "CCB Object Modify, > >> saLogStreamFixedLogRecordSize=80, strC"); > >> + test_case_add(4, saLogOi_30, "CCB Object Modify, > >> saLogStreamFixedLogRecordSize=150, strC"); > >> test_case_add(4, saLogOi_31, "immlist strA-strC"); > >> test_case_add(4, saLogOi_32, "immfind strA-strC"); > >> test_case_add(4, saLogOi_33, "saflogger, writing to notification"); > >> @@ -3440,6 +3457,7 @@ void verAdminOpOnConfClass(void) > >> test_case_add(6, saLogOi_110, "Modify: > >> saLogStreamFixedLogRecordSize == 0, Ok"); > >> test_case_add(6, saLogOi_111, "Modify: > >> saLogStreamFixedLogRecordSize == logMaxLogrecsize, Ok"); > >> test_case_add(6, saLogOi_112, "Modify: > >> saLogStreamFixedLogRecordSize > logMaxLogrecsize, ERR"); > >> + test_case_add(6, verFixLogRec_Min, "Modify: > >> saLogStreamFixedLogRecordSize < 150, ERR"); > >> test_case_add(6, saLogOi_113, "Modify: > >> saLogStreamMaxFilesRotated < 128, Ok"); > >> test_case_add(6, saLogOi_114, "Modify: > >> saLogStreamMaxFilesRotated > 128, ERR"); > >> test_case_add(6, saLogOi_115, "Modify: > >> saLogStreamMaxFilesRotated == 128, ERR"); > >> diff --git a/tests/logsv/tet_saLogStreamOpen_2.c > >> b/tests/logsv/tet_saLogStreamOpen_2.c > >> --- a/tests/logsv/tet_saLogStreamOpen_2.c > >> +++ b/tests/logsv/tet_saLogStreamOpen_2.c > >> @@ -429,6 +429,108 @@ done: > >> } > >> } > >> > >> +/** > >> + * Add test cases to verify #1466 > >> + * maxLogRecordSize must be in range [150 - MAX_RECSIZE] if not 0. > >> + */ > >> + > >> +/** > >> + * Verify that setting maxLogRecordSize > MAX_RECSIZE (65535) is not > >> allowed. > >> + */ > >> +void verFixLogRec_Max_Err(void) > >> +{ > >> + SaAisErrorT rc = SA_AIS_OK; > >> + SaNameT logStreamName = {.length = 0 }; > >> + > >> + strcpy((char *)logStreamName.value, > SA_LOG_STREAM_APPLICATION1); > >> + logStreamName.length = strlen((char *)logStreamName.value); > >> + > >> + SaLogFileCreateAttributesT_2 appLogFileCreateAttributes; > >> + > >> + /* App stream information */ > >> + appLogFileCreateAttributes.logFilePathName = "appstream"; > >> + appLogFileCreateAttributes.logFileName = "appstream"; > >> + appLogFileCreateAttributes.maxLogFileSize = > >> DEFAULT_APP_LOG_FILE_SIZE; > >> + appLogFileCreateAttributes.maxLogRecordSize = 65535 + 1; > >> + appLogFileCreateAttributes.haProperty = SA_TRUE; > >> + appLogFileCreateAttributes.logFileFullAction = > >> SA_LOG_FILE_FULL_ACTION_ROTATE; > >> + appLogFileCreateAttributes.maxFilesRotated = 4; > >> + appLogFileCreateAttributes.logFileFmt = NULL; > >> + > >> + rc = saLogInitialize(&logHandle, &logCallbacks, &logVersion); > >> + if (rc != SA_AIS_OK) { > >> + fprintf(stderr, "Failed at saLogInitialize: %d\n ", (int)rc); > >> + test_validate(rc, SA_AIS_OK); > >> + return; > >> + } > >> + > >> + rc = saLogSelectionObjectGet(logHandle, &selectionObject); > >> + if (rc != SA_AIS_OK) { > >> + fprintf(stderr, "Failed at saLogSelectionObjectGet: %d \n ", > (int)rc); > >> + test_validate(rc, SA_AIS_OK); > >> + goto done; > >> + } > >> + > >> + rc = saLogStreamOpen_2(logHandle, &logStreamName, > >> &appLogFileCreateAttributes, > >> + SA_LOG_STREAM_CREATE, SA_TIME_ONE_SECOND, > >> &logStreamHandle); > >> + test_validate(rc, SA_AIS_ERR_INVALID_PARAM); > >> + > >> +done: > >> + rc = saLogFinalize(logHandle); > >> + if (rc != SA_AIS_OK) { > >> + fprintf(stderr, "Failed to call salogFinalize: %d\n", (int) rc); > >> + } > >> +} > >> + > >> + > >> +/** > >> + * Verify that setting maxLogRecordSize > MAX_RECSIZE (65535) is not > >> allowed. > >> + */ > >> +void verFixLogRec_Min_Err(void) > >> +{ > >> + SaAisErrorT rc = SA_AIS_OK; > >> + SaNameT logStreamName = {.length = 0 }; > >> + > >> + strcpy((char *)logStreamName.value, > SA_LOG_STREAM_APPLICATION1); > >> + logStreamName.length = strlen((char *)logStreamName.value); > >> + > >> + SaLogFileCreateAttributesT_2 appLogFileCreateAttributes; > >> + > >> + /* App stream information */ > >> + appLogFileCreateAttributes.logFilePathName = "appstream"; > >> + appLogFileCreateAttributes.logFileName = "appstream"; > >> + appLogFileCreateAttributes.maxLogFileSize = > >> DEFAULT_APP_LOG_FILE_SIZE; > >> + appLogFileCreateAttributes.maxLogRecordSize = 150 - 1; > >> + appLogFileCreateAttributes.haProperty = SA_TRUE; > >> + appLogFileCreateAttributes.logFileFullAction = > >> SA_LOG_FILE_FULL_ACTION_ROTATE; > >> + appLogFileCreateAttributes.maxFilesRotated = 4; > >> + appLogFileCreateAttributes.logFileFmt = NULL; > >> + > >> + rc = saLogInitialize(&logHandle, &logCallbacks, &logVersion); > >> + if (rc != SA_AIS_OK) { > >> + fprintf(stderr, "Failed at saLogInitialize: %d\n ", (int)rc); > >> + test_validate(rc, SA_AIS_OK); > >> + return; > >> + } > >> + > >> + rc = saLogSelectionObjectGet(logHandle, &selectionObject); > >> + if (rc != SA_AIS_OK) { > >> + fprintf(stderr, "Failed at saLogSelectionObjectGet: %d \n ", > (int)rc); > >> + test_validate(rc, SA_AIS_OK); > >> + goto done; > >> + } > >> + > >> + rc = saLogStreamOpen_2(logHandle, &logStreamName, > >> &appLogFileCreateAttributes, > >> + SA_LOG_STREAM_CREATE, SA_TIME_ONE_SECOND, > >> &logStreamHandle); > >> + test_validate(rc, SA_AIS_ERR_INVALID_PARAM); > >> + > >> +done: > >> + rc = saLogFinalize(logHandle); > >> + if (rc != SA_AIS_OK) { > >> + fprintf(stderr, "Failed to call salogFinalize: %d\n", (int) rc); > >> + } > >> +} > >> + > >> extern void saLogStreamOpenAsync_2_01(void); > >> extern void saLogStreamOpenCallbackT_01(void); > >> extern void saLogWriteLog_01(void); > >> @@ -505,5 +607,7 @@ extern void saLogStreamClose_01(void); > >> test_case_add(2, saLogStreamClose_01, "saLogStreamClose OK"); > >> test_case_add(2, saLogStreamOpen_2_46, "saLogStreamOpen_2 with > >> maxFilesRotated = 0, ERR"); > >> test_case_add(2, saLogStreamOpen_2_47, "saLogStreamOpen_2 with > >> maxFilesRotated = 128, ERR"); > >> + test_case_add(2, verFixLogRec_Max_Err, "saLogStreamOpen_2 with > >> maxLogRecordSize > MAX_RECSIZE, ERR"); > >> + test_case_add(2, verFixLogRec_Min_Err, "saLogStreamOpen_2 with > >> maxLogRecordSize < 150, ERR"); > >> } > >> ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
