DJGPP adjustments #1050
DJGPP adjustments #1050
Conversation
| # if defined(OPENSSL_SYS_MSDOS) && !defined(OPENSSL_SYS_WINDOWS) | ||
| # include <libc/unconst.h> | ||
| static void dosify_filename(const char *filename); | ||
| # endif |
There was a problem hiding this comment.
I'd argue that this is definition of bad idea. First of all #if condition is totally misleading. Because it's really something specific to DJGPP, which is effectively a POSIX-like environment which happens to run under DPMI. It's not really DOS and mentioning that it's not Windows either doesn't make it clearer. This is really case when compiler pre-define such as __DJGPP__ does much better job.
Secondly [and more importantly!] breaking const contract is hardly appropriate. It might work in specific situation (like our test suite), but it's error-prone in general case. And real trouble is that in latter case it would have to be treated as bug. Which means that one effectively introduces a bug here. Which "general case"? Well, on normal OSes attempt to write to const might actually result in SEGV. I don't know if DJGPP has notion of read-only pages, but even if it doesn't, programmer who passes string to function declared as accepting const one is entitled to expect that it doesn't change. Let's say it's passed later to some other program component and original format is the one to be used... Right thing to do is to allocate temporary buffer (on stack?), copy filename, "dosify" it, fopen and return file descriptor. And to minimize clobber in common source it would probably be appropriate to implement it in separate file, and override calls for fopen with say __djgpp_fopen by means of some pre-processor magic. Extra -D in config might be appropriate, and separate file can be injected through say uplink_aux_src? Note that in such case one would probably be able to remove all these #ifs, which is desirable, isn't it? Minimizing #if spaghetti that is.
There was a problem hiding this comment.
Yes, good point all around. And I like the idea of an auxiliary file, but why uplink? I'd rather make a new one, say bio_aux_src
There was a problem hiding this comment.
why uplink?
Solely because it's already there, so one doesn't need to make up more knobs...
On additional note, why not use the opportunity break out target to separate file in Configuraions. It's not like we plan to regularly perform regression tests on the platform in question, is it?
[For public reference. Last sentence is kind of a echo of internal discussion we had regarding what belongs in 10-main.cf and why would one single out single platform like effectively suggested above. My rationale is that sometimes we don't actually know if specific config is working and have to make an assumption. Or rather refuse to make one and simply go with status-quo of not knowing. But once we get to learn something about platform that we are not [ever] testing ourselves, learn by talking to people, we shouldn't feel bad about making the said assumption on per-platform basis and denote this fact by moving platform out of 10-main.cf.]
There was a problem hiding this comment.
Hmmm... come to think of it, considering file_fopen is already an #if sallad, I'm not sure if suddenly using a radically different solution for DJGPP makes sense right now. In that case, what's currently in file_fopen should be refactored in a similar manner... and that's a bigger job, which I'd make post 1.1.0.
However, the DJGPP solution in this PR isn't quite right, as you remarked... Working on it.
There was a problem hiding this comment.
considering file_fopen is already an #if sallad, I'm not sure if suddenly using a radically different solution for DJGPP makes sense right now
I'd say that creating meaningful precedent is not inappropriate at any time. And I don't really see that it would be bigger job, but let it be your call...
DJGPP is a 3rd party configuration, we rely entirely on the OpenSSL to help us fine tune and test. Therefore, it's moved to its own config.
levitte
force-pushed
the
fix-djgpp-20160510-1
branch
from
6c68413
to
5cdd797
Compare
|
I just pushed a modification that does away with quite a bit of what @dot-asm commented on. |
| char *iterator; | ||
| char lastchar; | ||
|
|
||
| newname = OPENSSL_malloc(strlen(filename)); |
There was a problem hiding this comment.
OPENSSL_malloc(strlen(filename)+1);
* Configure: Replaced -DTERMIO by -DTERMIOS in CFLAGS. * crypto/bio/bss_dgram.c [WATT32]: Remove obsolete redefinition of function names: sock_write, sock_read and sock_puts. * crypto/bio/bss_sock.c [WATT32]: For Watt-32 2.2.11 sock_write, sock_read and sock_puts are redefined to their private names so their names must be undefined first before they can be redefined again. * crypto/bio/bss_file.c (file_fopen) [OPENSSL_SYS_MSDOS]: Call dosify_filename to replace leading dot if file system does not support it. (dosify_filename): Replace leading dot in passed file name if file system does not support LFN. Replace all leading dots in the dirname part and the basname part of the file name. * e_os.h [__DJGPP__]: Undefine macro DEVRANDOM_EGD. Neither MS-DOS nor FreeDOS provide 'egd' sockets. New macro HAS_LFN_SUPPORT checks if underlying file system supports long file names or not. Include sys/un.h. Define WATT32_NO_OLDIES. * INSTALL.DJGPP: Update URL of WATT-32 library. Submitted by Juan Manuel Guerrero <juan.guerrero@gmx.de> RT#4217
OPENSSL_SYS_MSDOS is defined for Windows builds as well, so purely MSDOS code needs to be guarded with:
File path rewriting (to replace leading '.' in file path components with '_') was implemented by modifying a const string. While this probably does work on DOS today, it doesn't set a good precedent. Instead, we just insert a chunk of code that allocates a new string and copies with some modifications and use the result as a file path.
levitte
force-pushed
the
fix-djgpp-20160510-1
branch
from
5e461b2
to
dc0ef5d
Compare
| *iterator = *filename; | ||
| } | ||
| lastchar = *filename; | ||
| } |
There was a problem hiding this comment.
... and it looks like newname needs to be terminated with '\0'?
|
Looks proper. It would be optimal to hear from OP, but I'd plus-one it as is. |
|
I'll note the plusone. In the ticket, I've said that I'll wait for a response from OP 'til the end of the week. |
|
I just got confirmation, in RT#4217, that the changes work flawlessly. Plusone noted above, I'll squash and merge. |
|
.... and merged. Thanks for the review |
function names: sock_write, sock_read and sock_puts.
sock_read and sock_puts are redefined to their private names so
their names must be undefined first before they can be redefined
again.
dosify_filename to replace leading dot if file system does not
support it.
(dosify_filename): Replace leading dot in passed file name if file
system does not support LFN. Replace all leading dots in the dirname
part and the basname part of the file name.
FreeDOS provide 'egd' sockets.
New macro HAS_LFN_SUPPORT checks if underlying file system supports
long file names or not.
Include sys/un.h.
Define WATT32_NO_OLDIES.
Submitted by Juan Manuel Guerrero juan.guerrero@gmx.de
RT#4217