app_isdir() cleanup #3991
app_isdir() cleanup #3991
Conversation
I think it's better to use `GetFileAttributes` to obtain the attributes of a file than `FindFirstFile`. If the input name contains `*`, this function should return failure rather than check whether the first match happens to be a file or a directory.
openssl-machine
added
the
hold: cla required
richsalz
removed
the
hold: cla required
apps/apps.c
Outdated
|
|
||
| if (len_0 > OSSL_NELEM(FileData.cFileName)) | ||
| if (len_0 > MAX_PATH) | ||
| return -1; | ||
|
|
||
| # if !defined(_WIN32_WCE) || _WIN32_WCE>=101 | ||
| if (!MultiByteToWideChar |
There was a problem hiding this comment.
If you modify code that was malformed in grand reformat, it's appropriate to fix it at once. In this case
if (!MultiByteToWideChar(CP_ACP, 0, ...)
|
|
||
| if (len_0 > OSSL_NELEM(FileData.cFileName)) | ||
| if (len_0 > MAX_PATH) |
There was a problem hiding this comment.
Formally speaking there is no reason to hold on to the MAX_PATH limitation. I mean in FindFirstFile you have no choice, but here you do. But in this case, I'd say it's ok...
apps/apps.c
Outdated
| return -1; | ||
|
|
||
| # if !defined(_WIN32_WCE) || _WIN32_WCE>=101 | ||
| if (!MultiByteToWideChar | ||
| (CP_ACP, 0, name, len_0, FileData.cFileName, len_0)) | ||
| (CP_ACP, 0, name, len_0, tempname, len_0)) |
There was a problem hiding this comment.
On side note formally speaking there is a logical inconsistency here. The call assumes that it takes equal number of characters, len_0, to represent name. But there is no guarantee that it's the case. However, it usually takes less WCHARs, so overall it works out...
There was a problem hiding this comment.
I think it's guaranteed that number of WCHARs is less than or equal to number of chars. If one Unicode character requires two WCHARs (i.e. 4 bytes) in UTF-16, its code point is at least 0x10000. I don't think there's any encoding scheme using one byte to represent such a character.
Or do you want me to change the second len_0 to MAX_PATH?
There was a problem hiding this comment.
Or do you want me to change the second len_0 to MAX_PATH?
"Side note" means that no action is expected. But it doesn't mean that you can't ponder over it. You might get a viable idea. And suggestion to replace len_0 with MAX_PATH can be viewed as readability improvement... Your call :-)
| return -1; | ||
| FindClose(hList); | ||
| return ((FileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0); | ||
| return ((attr & FILE_ATTRIBUTE_DIRECTORY) != 0); |
There was a problem hiding this comment.
Coding style guide suggests that minimum parenthesis are preferred with return. At the same time it's not directly inappropriate to put complex or long expressions into parenthesis by themselves. So I'm going to let it slide in this case, but second reviewer might have different opinion. Even though commits will be squashed when it goes to repository, it's only appropriate to keep follow-ups in the merge requests as separate small commits. I'm also adding 1.1.0 label, because it's kind of bug.
dot-asm
added
1.1.0
branch: master
|
Actually it's not inappropriate to apply it even to 1.0.2, so I'm adding even that label. |
I think it's better to use `GetFileAttributes` to obtain the attributes of a file than `FindFirstFile`. If the input name contains `*`, this function should return failure rather than check whether the first match happens to be a file or a directory. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #3991)
I think it's better to use `GetFileAttributes` to obtain the attributes of a file than `FindFirstFile`. If the input name contains `*`, this function should return failure rather than check whether the first match happens to be a file or a directory. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #3991) (cherry picked from commit 5bd051a)
|
Squashed and merged to master and 1.1.0. Thanks! |
I think it's better to use `GetFileAttributes` to obtain the attributes of a file than `FindFirstFile`. If the input name contains `*`, this function should return failure rather than check whether the first match happens to be a file or a directory. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from openssl#3991) (cherry picked from commit 5bd051a)
I think it's better to use
GetFileAttributesto obtain the attributesof a file than
FindFirstFile. If the input name contains*, thisfunction should return failure rather than check whether the first match
happens to be a file or a directory.