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 > 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. 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
