fix #320, fix lgtm osal issue #326
Conversation
| @@ -191,8 +192,15 @@ int32 OS_FileChmod_Impl(const char *local_path, uint32 access) | |||
| * which is generally not part of the OSAL API, but | |||
| * is important for the underlying OS. | |||
| */ | |||
| if ( stat(local_path, &st) < 0 ) | |||
| fd = open(local_path, O_RDONLY); | |||
There was a problem hiding this comment.
Opening the file with O_RDONLY here changes behavior, though, for any file which does not have read permission. Files can have write-only permission or no permission at all. stat and chmod handle those cases just fine, but open() here will fail.
Also note that OS_FileChmod can be used to remove read permission on a file, and this prevents it from ever adding it back again.
I looked into this a bit, there is a O_PATH flag to open() that seems to address this issue, but it is Linux-specific (non-posix).
| @@ -214,7 +214,8 @@ int32 OS_SocketBind_Impl(uint32 sock_id, const OS_SockAddr_t *Addr) | |||
| break; | |||
| } | |||
|
|
|||
| if (addrlen == 0 || addrlen > OS_SOCKADDR_MAX_LEN) | |||
|
|
|||
| if (addrlen == 0 || addrlen > OS_SOCKADDR_MAX_LEN) // lgtm [cpp/constant-comparison] | |||
There was a problem hiding this comment.
It might be possible to use some form of assert (either compile time or run time) to check that the sizes of the address structures are less than OS_SOCKADDR_MAX_LEN, then this runtime test can be removed, rather than adding markup here.
| { | ||
| return OS_ERR_NOT_IMPLEMENTED; | ||
| } | ||
|
|
||
| Addr->ActualLength = addrlen; | ||
| Accessor->sockaddr.sa_family = sa_family; | ||
| Accessor->sockaddr.sa_family = sa_family; // lgtm[cpp/uninitialized-local] |
There was a problem hiding this comment.
Suggest just initializing this to 0 at the start of the function. Should avoid the false warning.
| @@ -327,7 +327,7 @@ static int32 OS_FileSys_Initialize(char *address, const char *fsdevname, const c | |||
| { | |||
| /* to avoid leaving in an intermediate state, | |||
| * this also stops the volume if formatting failed. */ | |||
| OS_FileSysStopVolume_Impl(local_id); | |||
| OS_FileSysStopVolume_Impl(local_id); // lgtm[cpp/useless-expression] | |||
There was a problem hiding this comment.
On second look maybe the OS_FileSysStopVolume_Impl shouldn't be entirely empty here.
| @@ -526,7 +526,7 @@ static int32 OS_ObjectIdFindNext(uint32 idtype, uint32 *array_index, OS_common_r | |||
|
|
|||
| if(return_code == OS_SUCCESS) | |||
| { | |||
| return_code = OS_ObjectIdMap(idtype, idvalue, &obj->active_id); | |||
| return_code = OS_ObjectIdMap(idtype, idvalue, &obj->active_id); // lgtm[cpp/uninitialized-local] | |||
There was a problem hiding this comment.
All of these are false alarms but could probably be avoid by setting both obj = NULL and local_id = 0 at the start of the function.
| @@ -269,7 +269,7 @@ int32 OS_SocketAccept(uint32 sock_id, uint32 *connsock_id, OS_SockAddr_t *Addr, | |||
| OS_SocketAddrInit_Impl(Addr, OS_stream_table[local_id].socket_domain); | |||
|
|
|||
| /* The actual accept impl is done without global table lock, only refcount lock */ | |||
| return_code = OS_SocketAccept_Impl(local_id, conn_id, Addr, timeout); | |||
| return_code = OS_SocketAccept_Impl(local_id, conn_id, Addr, timeout); // lgtm[cpp/uninitialized-local] | |||
There was a problem hiding this comment.
Again, suggest setting to 0 at top of function to avoid this false alarm, rather than a markup comment
| @@ -463,7 +463,7 @@ int32 OS_TimerDelete(uint32 timer_id) | |||
| */ | |||
| if (return_code == OS_SUCCESS) | |||
| { | |||
| OS_ObjectIdRefcountDecr(timebase); | |||
| OS_ObjectIdRefcountDecr(timebase); // lgtm[cpp/uninitialized-local] | |||
There was a problem hiding this comment.
Suggest setting timebase = NULL at start to avoid this false alarm
|
split pull request. |
Describe the contribution
Resolve lgtm issue
Testing performed
Steps taken to test the contribution:
System(s) tested on:
Contributor Info
Anh Van, NASA Goddard