WIP: quotes-fix #344
mxlgv
Reference in New Issue
Block a user
Delete Branch "Matou1306/kolibrios:quotes-fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #336:
Created a new asm template "quotkinizer" that takes a path and wraps it in quotes.
This file sits in programs (so that specific wrapper binaries could be created for any program where needed), for dgen specifically: I made dgen_open.asm which includes the quotkinizer template ... This dgen_open is now the main binary linked in assoc.ini to open .md and .gen files. All what dgen_open does is execute dgen but with quotes over the path
I tested it locally with a dummy .md file (with spaces all over the path) and verified that it didn't show the path truncation error. (However, it's stuck on dgen logo, I don't know if that's because it's a dummy file or what... but even before implementing the fix it was also getting stuck at the logo)
But the main error that I was getting: "Unable to load "/tmp0/1/New" stopped appearing when opening "/tmp0/1/New folder/New file.md"
6dd27a94f6toc8efe352dcc8efe352dctoe7d1a0b5dbI already wrote about what can be done. This solution, in my opinion, is bad.
@mxlgv Just to make sure we're on the same page, I realized the PR description was outdated (just updated it).
I completely removed the global macro and the @open changes. The current code only modifies dgen/main.c to handle arguments locally.
If you already reviewed this latest committed code and still see issues, could you please give more specific details on what's wrong?
@Matou1306 #336 (comment) here I wrote that you can make a launcher specifically for working with files. The launcher is a simple fasm program that adds quotes to the path and launches the program. Thus, changes to the port are not necessary.
@mxlgv Oh my bad!! I just realized I totally missed this comment from last week! I had the page open so it didn't send me a notification.
I really appreciate your apology from that message, and sorry for the confusion I caused by missing this for a whole week while you had already pointed out the preferred path.
Thanks for referencing it, I will discard the past changes and start working on your approach then push the edits asap
quotes-fixto WIP: quotes-fix@Matou1306 Thank you for showing such interest in the project)
e7d1a0b5dbtod8d4759097@mxlgv I implemented your suggested FASM wrapper and pushed the latest version. Please take a look and tell me your thoughts
(Note: I tested it with a dummy file with spaces in the path. The path truncation error is completely gone. It hangs on the DGen logo though but it did that before the fix too (since it's not a valid file I guess)).
Waiting for your comments and review.
You took writing the template too literally, I suggest giving up inc and making a program in a separate folder.
I suggest that it be universal. To have the program automatically launch a file in its folder with the same name without
open: for example/kolibrios/blabalbal/dgen_openwill launch/kolibrios/blablabal/dgen.@@ -0,0 +3,4 @@format binary as ""TARGET equ '/kolibrios/emul/dgen/dgen'It's a bad decision to hardcode the file path
@@ -0,0 +21,4 @@mov edi, new_paramsmov byte [edi], '"'inc ediWhy are you copying the path? It is better to hardcode the quote before the path and add the last quote to the end of the line.
@@ -0,0 +37,4 @@mov dword [fi + 8], new_params.launch:mov eax, 70Use constants from KOSfunc.inc.
@Matou1306
What's the bug in DGEN? Which specific file is not valid. Does DGEN run a file with quotes via SHELL for example?
@mxlgv Sorry for the delay, I had university exams today.
Regarding your question: initially, no, I didn't try it via SHELL. I just double-clicked the file from Eolite and got DGen opening but stuck on the logo. The file I used is a dummy file (just an empty/random char filled New file.md). I don't actually know of a valid file/ROM that works on DGen to test this properly (help finding one to test would be appreciated).
I just tried using SHELL per your suggestion. After some trial and error, the final result I got was: 'dgen' started. PID = 31, but no actual window opened.
And regarding your requested changes:
-Do you mean something like ../../? Or by universal you mean something even more general like dynamically reading the file's own executable path? Like for dgen it should read
/kolibrios/emul/dgen_open, chop off _open and launch/kolibrios/emul/dgen?-My original intention was to avoid memory access violations in case the path didn't have a free byte in the beginning, but yeah I realized it would already be O(n) time to reach the null terminator, so maybe just shifting it while parsing would be a better solution by avoiding O(n) space too?
-Regarding KOSfunc.inc, noted.
I will start working on these updates and push them asap
Hey @mxlgv, I just pushed your requested changes. Please take a look and let me know if there is anything else.
The only thing remaining is the relative path change you requested. I am not totally sure I understand your plan correctly on this one, so I am waiting for your answer to my previous question before making this specific change.
@Matou1306 Hi!
Yes:
Regarding the quote.
If I'm not mistaken, then according to the documentation, the argument cannot be larger than 256 bytes (including '\0'). I suggest using this size for now.
What was approximately expected:
A separate program #344 (comment) that works approximately on this principle:
and
and
and RUN.
@mxlgv Hello,
Thanks for the review. I implemented your requested changes, please take a look and tell me what else I can improve.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.