Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows compatibility fixes #6

Closed
wants to merge 132 commits into from
Closed

Conversation

SteveFosdick
Copy link
Collaborator

Thomas,

Sorry to go back over old ground but can we go back to using the complete GNU version of tsearch, i.e. the tsearch.c file, not just add a tdestroy function to compat_wrappers.c? The tdestroy function is not an independent function - it assumes something about the data structures built by the other functions in tsearch and the only time we can be sure the implementation of tsearch uses those exact data structures in when the C library for the platform is the GNU library which is the one case we don't need to supply tdestroy.

So, by definition, if we are having to provide an implementation of tdestroy for a particular environment we would do better to use a consistent set of functions rather than trust what is in the C library. These functions don't interact with the OS in any way, they just maintain a data structure.

Another portability change here is to provide stpcpy which it seems WIndows does not have. That is a small independent function so I have added that one to compat_wrappers.c. I have also had to turn off -Werror (again) as having it on stops the autoconf test for stpcpy from finding it. This is because it runs the C compiler to check for the function with the same flags as the project is built with, but doesn't generate code for the test that is good enough to avoid warnings.

There is a change to vdfs.c to avoid using POSIX file permission constants that again it seems windows does not have. Finally, with regard to replacing most fopen calls with x_fopen failing to load or save the CMOS ram isn't really fatal so it is reasonable for the emulator to continue in that case rather than stopping with an error.

hoglet67 and others added 30 commits March 16, 2016 19:27
Change-Id: Idf6e706650e0e49a0c4592e7cf009a1192555618
@ThomasAdam
Copy link
Contributor

@SteveFosdick -- I'm fine with tsearch for now, but I am going to be switching away from it at some point to use different data structures. You need to ensure though that you git add src/tsearch.c and related files -- you've referenced them in Makefile.am but I suspect forgot to include it in the commit.

As for stpcpy... OK, again for now, but I am about to audit b-em for its string-handling use, since it's getting a little unwieldy given the recent introduction of src/logging.c and there's no reason why we can't standardise this a lot better -- especially when it comes to supporting windows as well.

@SteveFosdick
Copy link
Collaborator Author

@ThomasAdam,

Regarding tsearch I am not attached to that implementation but I did chose that API because it did what I wanted. What do you propose to replace it with? Does it involve an external library dependency? The source file tsearch.c is in my master and git status says it is up-to-date. tsearch.c is not listed as an untracked file and git log reports some history on it.

What is the motivation to audit the string handling? Buffer underun bugs?

@SteveFosdick
Copy link
Collaborator Author

P.S. Are the conflicts on the 32016 code because you have merged hoglet's latest 32016 code ahead of this pull request? If so I'd just stick with his code - I am not doing anything 32016 relalated.

@hoglet67
Copy link
Contributor

Something seems to be badly wrong with this pull request, as the commit IDs of all these existing commits has changed. Hence this looks like a monster pull request with thousands of changes.

e.g. Add a "counted continue", i.e. continue until the specified number of breakpoints are hit:

  • in master this is 9a98686
  • in the pull request it is 4cfbf82

Has some rebasing been going on?

@ThomasAdam
Copy link
Contributor

@hoglet67 -- it's because @SteveFosdick has been merging in changes from master and not rebasing before sending out the updates. It's not a problem, as I'm not using this pull-request directly to do the merging, as there was originally a lot of things to fix up. So all's well.

@hoglet67
Copy link
Contributor

@ThomasAdam it might help greatly if you were to outline the recommended pattern here, as I've not worked a great deal with pull requests. What I'm not clear on is when exactly to rebase in preparing a pull request, and what this is trying to achieve.

@ThomasAdam
Copy link
Contributor

@hoglet67 -- Sure, give me a second and I'll push something to master -- I've already documented this for other projects so it's reusable.

@ThomasAdam
Copy link
Contributor

@SteveFosdick @hoglet67 -- try this: https://github.com/stardot/b-em/blob/master/SUBMITTING_PATCHES.md

Let me know if there's anything there which isn't clear, or needs changing, etc.

@hoglet67
Copy link
Contributor

Thanks Thomas, that's very helpful.

@ThomasAdam
Copy link
Contributor

@SteveFosdick -- I'm not sure which tsesarch implementation you chose, but can you have a look at it, please? Since I'm on a system which has to make use of the one we're trying to bundle in, I'm getting these errors when trying to compile tsearch.c:

tsearch.c:698:30: error: unknown type name '__action_fn_t' 
trecurse (const void *vroot, __action_fn_t action, int level)                                                          
                             ^                             
tsearch.c:721:27: error: unknown type name '__action_fn_t' 
twalk (const void *vroot, __action_fn_t action)            
                          ^                                
tsearch.c:735:30: error: unknown type name '__free_fn_t'   
tdestroy_recurse (node root, __free_fn_t freefct)          
                             ^                             
tsearch.c:747:24: error: unknown type name '__free_fn_t'   
tdestroy (void *vroot, __free_fn_t freefct)                
                       ^                                   
4 errors generated.                                        
*** Error code 1                                           

That will be because both ``__action_fn_tand__free_fn_t` are tsearch-specific, but the implementation doesn't define them anywhere.

Any thoughts?

@SteveFosdick
Copy link
Collaborator Author

I am on the phone without my glasses bu the action one is probably the callback from twalk so this will be

void (*action)(const void *nodep,
const VISIT which,
const int depth));

The free one is the one in compact_wrapper.h which should match:
void (*free_node)(void *nodep)

The implementation is from the latest glibc.

Hope that makes sense.

@ThomasAdam
Copy link
Contributor

ThomasAdam commented Feb 25, 2017

@SteveFosdick -- things like tree.h (which are just a series of macros) which defines things like red/black trees, tail queues, circle queues, etc. The performance of them is good, and I've used them a lot in other projects. The portability is easily mitigated by shipping tree.h and using it if a known function isn't defined; same as any AC_CHECK_FUNC call.

As for string auditing -- no, not buffer problems, but how we're using some of the C lib calls:

  • stpcpy
  • snprintf
  • asprintf

etc., and where necessary making those more uniform.

@ThomasAdam
Copy link
Contributor

@SteveFosdick -- hmm, unless I'm missing something, there isn't anything in compat_wrapper.h related to tsearch -- equally, VISIT is something which would also need to be defined.

This needs further investigation, I think. I'm happy to do it, but it will mean copying code from somewhere to support this.

@SteveFosdick
Copy link
Collaborator Author

@ThomasAdam

On the question of rebasing I'll admit although I use git locally I haven't really used it as a collaboration tool. I have read you submitting patches document and it is still as clear as mud. I think I'll have to read more more of the it documentation.

On the question of tsearch, how are you getting on? I could propose some changes to fix this but as I am on a platform that has the GNU search.h header anyway that isn't going to be the best test of whether that would work elsewhere. A quick look suggests the missing typedefs are to be found in the misc/search.h file in the GNU C library distribution which I got from http://ftp.gnu.org/gnu/glibc/glibc-2.25.tar.xz. Let me know if you do want to to submit something.

For the longer term, tree.h sounds interesting - could you send me a link to it, please?

On the question of string library calls it seems to me there are different ones because they serve a different purpose. So, for example, you would not replace a call to snprintf where the result is used only temporarily, the maximum length of the result is predicable (unless the maximum length is long and occurs relatively infrequently), with a call to asprintf which incurs the overhead of malloc and requires check that the string is then freed again. On the other hand if the result is returned from a function then heap storage is the right answer and stack storage will have been reclaimed and static variables are not thread-safe.

@ThomasAdam
Copy link
Contributor

ThomasAdam commented Feb 27, 2017

@SteveFosdick -- that's fine. I figured there'd be a few teething problems with various contributors trying to use Git for the first time. If there's something I can help you with, or to better explain some concepts, then just drop me an email. As for tree.h, see: https://github.com/tmux/tmux/blob/master/compat/tree.h which is what I've been carrying around portably now for some time, it works really well.

@ThomasAdam
Copy link
Contributor

Merged this in, with changes from me for tsearch, and other minor things.

Thanks, @SteveFosdick!

@ThomasAdam ThomasAdam closed this Mar 1, 2017
SteveFosdick pushed a commit that referenced this pull request Mar 17, 2020
@SteveFosdick SteveFosdick mentioned this pull request Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants