let's deal with remaining bugs in dopus5 together

While Itix are busy and Xenic on vacation, i am porting all the dopus5 modules, with all your help fix __alignment issues, few bugs here and here , but still there is some left, which i do not want to hold till Ikka/Xenic will have time to look at, so maybe it will be better and faster if i will point out on them, show the code, and you all can help to fix those bugs.

Let's start from the first one. It is 2 bugs in filetype.module. One of them is our-port-os4-native (do not happens on ported os3 version). Another one are coming in module by default (i.e. even original os3-sasc version have that bug. can be as bug in the original code, and as differences between os3/os4)

There is source file of filetype module:
https://sourceforge.net/p/dopus5allami ... iletype_module/filetype.c

(there is some more, but in general all main code of module here).

So, first bug should be more or less easy one: garbled characters in some parts. Check screenshot.



You can see in small filetype_creator window fancy characters like something wrong with copy of strings buffer or so.

Second bug which come in original code, is happens like this:

1. run dopus5
2. dbl-click on any .guide file, sniffer will popup, choice "Sniff!"
3. in new window choice "create"
4. in new window choice "save"
5. you will have now window "filetype 'untitile' exists. do you wish to replace it ?" : press Chancel. Title screen bar will blink by black color, like there no action was.
6. press "chancel" in filetype creator window = > GR with such stack trace:


Stack trace:
(0x637B2E40) [config_close.c:189] L_FreeFiletypeList()+0x2c (section 1 @ 0x35B44)
(0x637B2E50) [aos4_ppc_libstubs.c:925] libstub_L_FreeFiletypeList()+0x14 (section 1 @ 0x9020)
(0x637B2E60) [filetype.c:680] finder_creator_proc_code()+0x2d0 (section 1 @ 0x5B7C)
(0x637B2F90) native kernel module dos.library.kmod+0x00023434
(0x637B2FC0) native kernel module kernel+0x0006ac70
(0x637B2FD0) native kernel module kernel+0x0006acf0


DAR=0x00000000, what mean it's uninitiated list header (NULL).

filetype.c:680 code is just "FreeFiletypeList( ftl );", which is Library function "L_FreeFiletypeList", and code of which from config_close.c:189 of library looks like this:



I do test the same code on morphos, and there is no problems. Its reacts the same (i.e. blank blinks like nothing happens), but no crash when i press chancel in main window.

Any ideas/hints/help/or-better-ready-to-compile-code very welcome :)

Hi kas1e

Would love to help, but does this thing compile with a cross-compiler right now? If so then I can offer you some help with porting and compiling.

Regarding your second error below, has the list been initialised correctly with NewList()?

Let me check the source out and see if I can get a better look at it.

Daniel

..

Quote:

Yes, this is true, but I have very little spare time these days. I will try and help when I can but I can't commit to lots of time. That's just the way it is unfortunately.

@kas1e

If the list is unitilised node == NULL there fore node->ln_Succ = DSI on 0x00000000;

Chnage code to:



Note: change the for loop test to

node && node->ln_Succ

if node is NULL then the seond term is never evaulauted so no crash.

This was one of the most common bugs in AWeb at one point.


[edit]
Perhaps the list is not even allocated? In which case the node = list->filetype_list will cause the failure.

so do

You should not even be using the names "lh_Head", "ln_Succ" and others within the structures. The Exec provides foolproof calls to find the first/next/previous/last node in a List, and returns NULL safely if the item asked for does not exist.

What is going to happen when we change the contents of a list node? Your port is going to fail. Use the provided Exec calls like GetHead(), GetSucc(), etc, and remain future-proof.

The loop should read:

for (node = IExec->GetHead(filetype_list;
node != NULL;
node = IExec->GetSucc(node))
{
L_FreeFiletype((Cfg_Filetype *)node);
}

@Andy

If i do:



Then on first line it says:

Quote:

If i remove it, and keep only:



Then i have no hardcore crash , but memguard hit which point out on "FreeVec(6FF6B0A8 [, 0]) from filetype.module itself. Maybe is this FreeFiletypeList( ftl ); on line 680 of filetype.c in filetype.module should be fixed instead of library function ?

@Tonyw

Your code bring bunch of errors:



And in general we need to keep it for os3/mos/aros as well, so the less specific changes the better.

Quote:

There is many places where dopus5 can fail if anything will changes. We need now bug-free version of what we already have, before adding and change anything in the code which can be not changed for a while and which can go to todo/improvements later

@kas1e

You know what Kas1e, sod it with trying to help you.

I've checked out, tidied up, and tested a sh*t load of code in that filetype.module source code and you're not even going to give me write-access to the repository "in case I break something".

I even refactored that 'Free Nodes' code that's already been mentioned above with proper GetSucc/GetHead etc.

Why ask for help when all you want me to do is send you source code fragments in an email or post them on to a forum. Even if I do break it, which is pretty much impossible, then the whole point of SVN is to roll back to a different version.

Good luck keeping your precious source code to yourself!

..

Quote:

I'm trying to help, by spending some of my very little, precious, spare time fixing source code for you!

Quote:

If you only keep "active" ones, who clearly have more spare time than some other people (who have 12 hour day full time jobs, families etc) then why come begging on a forum for help.

Do you pay me £60k a year to work on your source code? If you want to do so, then yeah, I'll quit my job and put the effort into your "projects".

If you want a bit of help now and again, which you're always asking for, to tidy source code, fix bugs and refactor small pieces of code to make the project better then I was willing to do that as and when I could.

Yes, it might not be as often as you liked, but that's the offer was, and that's the way it's got to be.

However, as I said in my PM, that offer is now rescinded!

Quote:

I'm not as stupid to commit something that I'm likely to break, I know fine well that adding a couple of GetHead/GetSucc calls is NOT going to break os3/mos/aros compatibility!

I fixed nigh-on 40 warnings, on top of other things, which wouldn't make the slightest difference to the compatibility.

What about all of the source code you blindly commit to the repository, copying and pasting from snippets of code posted on a forum?

On a final note, no one is crying, I'm way passed caring about your port now, so you can please yourself what you do with the source code. For me, as I have an interest in Magellan, I think I'll just work on a local OS4 version in my spare time to see what I can come up with.

Happy coding!

..

back on topic please or otherwise i will close the post. Use PM if you want discuss these kind of things.

..

yes it is possible but i won't do. i don't like to delete other posts. You can always edit your posts.

Because of your "discussion" with Slash (and the weird way this site only lets you see the last few posts in the thread when replying and not the actual post you are trying reply to!) I can't quote the relevant bits but:

The error about scaler verses struct is because I misread the code and the list->filetype_list is obviously an embedded list not a pointer to a list.

The fix I suggested makes the code more robust in case of error (hence no GR now), so you should keep it, but if you get a MemGuard hit then obviously there is an error further up the chain. It's not at all obvious from reading the code (in the time I have I can't do a build to add debug etc). I would suggest fixing the warnings if any and see what it shows. Examine them carefully before fixing, they may show the location of the bad memory allocation / freeing.

Quote:

This code is dangerous. You will note that the iteration statement of the for() loop uses the node pointer after you have freed it. Other problems aside, the original code had some protection against this. There are other loop constructs that would be better suited, but for minimal changes:

@Karlos

Thanks as well, but about all that GetHead/GetSucc and co, there is what ikka says about, and i kind of agree on this:
https://sourceforge.net/p/dopus5allami ... sion/dev/thread/82cdef52/

Relevant part:

Quote:

I.e. now it is no problem for make code looks "good". With all those replacements and other non-necessary stuff (right now it is not necessary for sure).

Of course even if someone will says that those macroses avail on 68k, that will be no differences, as point it just to not make work we do not need right now and to not jump on problems which we not have right now.

Our problems now its just about few bugs which need to be fixed before public release, without re-factoring of code totally, without rewriting parts which make no sense to rewrite now. All that crap can go for todo/requests.

@Andy
Yep, starting to fix warnings.

GetHead, GetTail, GetSucc and GetPred are all there for a reason. On OS4, it is highly likely the internal structure of a "node" may change in the future. With this in mind, accessing the fields of the structure will break any program when those changes are made. By using the API functions, your program will continue to work even after the node changes, it might even become a VOID *, who knows.

In any case, 68k is dead, so it makes no difference there, but on OS4 it's worth it to use the API. It goes without saying that creating a few macros for 68K source compatibility is a trivial job, and one that would be worth it in the log run.

I understand you want to "get it working" now, but in the long run you are creating more work, because someone is going to have to go through the code later and "fix" these issues.

Simon

@Rigo

Well if you do you need to recompile the program, macros wont replace them self.

Unless you want drop legacy you need a second method or new types of node and list structures that has northing to do whit old, that can live side by side of legacy.

@Karlos
Is it dangerous? Only if the list is shared, we don't know that from the what is posted here.

If its NOT a OS structure it should be protected by Mutex.

@Kas1e
If its a sheard and if its not protected its, a danger that the a node might be deleted, during the for loop, or one of the ln_Succ pointer gets changed during the for loop.