Hi Lennart, Thanks for your comments. Please see my response inline.
Regards, Vu >-----Original Message----- >From: Lennart Lund [mailto:[email protected]] >Sent: Tuesday, October 13, 2015 3:02 PM >To: Vu Nguyen M; [email protected]; Giang Do T >Cc: [email protected] >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:[email protected]] >> Sent: den 7 oktober 2015 09:28 >> To: [email protected]; Lennart Lund; Giang Do T >> Cc: [email protected] >> 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. > >> 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. 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 [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
