Implemented a wrapper for placing the path to a file in quotes. This corrects the problem of handling paths containing spaces in C programs such as DGEN, since the space is the argument delimiter.
The wrapper should have the name <app>_open. Wrapper trims the _open postfix and launches the <app>.
Fixes #336:
Implemented a wrapper for placing the path to a file in quotes. This corrects the problem of handling paths containing spaces in C programs such as `DGEN`, since the space is the argument delimiter.
The wrapper should have the name `<app>_open`. Wrapper trims the `_open` postfix and launches the `<app>`.
@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?
@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.
@Matou1306 https://git.kolibrios.org/KolibriOS/kolibrios/issues/336#issuecomment-4405 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
@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
mxlgv
marked the pull request as work in progress 2026-03-02 18:30:28 +00:00
@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.
@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_open will launch /kolibrios/blablabal/dgen.
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_open` will launch `/kolibrios/blablabal/dgen`.
@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
@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.
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.
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
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.
* Command line must be terminated by the character with the code 0
What was approximately expected:
A separate program #344 (comment) that works approximately on this principle:
q_path:; You can also not use a label but write path+1 in the MENUET header
rb1path:rb256+1; + 1 for end quote
and
moval,'"'mov[q_path],al
and
path[strlen(path)]='"';
and RUN.
@Matou1306 Hi!
Yes:
> 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
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.
https://git.kolibrios.org/KolibriOS/kolibrios/src/commit/6f2a947deb6add78781762d3d8cbad7bf1ef0e3d/kernel/trunk/docs/sysfuncs.txt#L4348
What was approximately expected:
A separate program https://git.kolibrios.org/KolibriOS/kolibrios/pulls/344#issuecomment-4582 that works approximately on this principle:
```asm
q_path: ; You can also not use a label but write path+1 in the MENUET header
rb 1
path:
rb 256+1 ; + 1 for end quote
```
and
```asm
mov al,'"'
mov [q_path], al
```
and
```c
path[strlen(path)] = '"';
```
and RUN.
I would recommend the mcall macro for system calls:
https://git.kolibrios.org/KolibriOS/kolibrios/src/commit/0253ec87530d26e76ee38ecb101d8839d6c354d0/programs/macros.inc#L297
@Matou1306
Forgot to remind you to add this there: https://git.kolibrios.org/KolibriOS/kolibrios/src/commit/42c205be32e7e55b7174b0aa79ea76b08c0db1cf/data/Tupfile.lua#L802
I believe it should be '/fs/open_wrap/open_wrap' on the right side.
By the way, if we rename the program to dgen_open, blah_open, etc, then maybe it should be called wrap_open, not open_wrap? Just a thought, not a requirement.
I believe it should be '/fs/open_wrap/open_wrap' on the right side.
By the way, if we rename the program to dgen_open, blah_open, etc, then maybe it should be called wrap_open, not open_wrap? Just a thought, not a requirement.
That path was depending on the name fs/wrap_open/Tupfile.lua created, I updated both.
I also renamed open_wrap to wrap_open like u said, it feels cleaner indeed.
That path was depending on the name `fs/wrap_open/Tupfile.lua` created, I updated both.
I also renamed `open_wrap` to `wrap_open` like u said, it feels cleaner indeed.
Dgen itself is an ancient Genesis/MegaDrive emulator, so that's what gen and md stand for. The .md extension here just means it's a Sega game image.
(Honestly I had no idea at the start too and I even once tried testing with markdown text lmao)
Dgen itself is an ancient Genesis/MegaDrive emulator, so that's what gen and md stand for. The .md extension here just means it's a Sega game image.
(Honestly I had no idea at the start too and I even once tried testing with markdown text lmao)
Since the final design of the program can be used to call any other program, not just dgen, there is no need to call its binary dgen_open. Let the binary be wrap_open here and then copy it to the dgen directory as dgen_open, to a potential blahblah directory as blahblah_open, etc.
Since the final design of the program can be used to call any other program, not just dgen, there is no need to call its binary dgen_open. Let the binary be wrap_open here and then copy it to the dgen directory as dgen_open, to a potential blahblah directory as blahblah_open, etc.
This was there since the start since at first I used to treat params as one single block. When dividing it I just added a virtual block. But like you said there is no difference in the output binary so I'll just keep it simple and do it as you suggested.
This was there since the start since at first I used to treat params as one single block. When dividing it I just added a virtual block. But like you said there is no difference in the output binary so I'll just keep it simple and do it as you suggested.
Hello, I just want to add a comment that I tested the changes locally yesterday with a real .md game file and they work fine:
I installed an old dgen game (Fix It Felix, Jr.) .md image file and tried opening the file on qemu. It worked just fine. I then tried adding spaces to the path and it also worked, meaning the fix worked and wrap_open handles everything perfectly.
Hello, I just want to add a comment that I tested the changes locally yesterday with a real .md game file and they work fine:
I installed an old dgen game (Fix It Felix, Jr.) .md image file and tried opening the file on qemu. It worked just fine. I then tried adding spaces to the path and it also worked, meaning the fix worked and wrap_open handles everything perfectly.
mxlgv
approved these changes 2026-03-23 14:30:53 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Burer
dunkaist
mxlgv
Fixes #336:
Implemented a wrapper for placing the path to a file in quotes. This corrects the problem of handling paths containing spaces in C programs such as
DGEN, since the space is the argument delimiter.The wrapper should have the name
<app>_open. Wrapper trims the_openpostfix and launches the<app>.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
@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.
I think I came up with a pretty stupid name... I suggest adding this AS A PROGRAM to
programs/fs/open_wrap/open_wrap.asm@@ -0,0 +5,4 @@virtualdb WRAPPER_SUFFIXSUFFIX_SIZE = $ - $$I think this is unnecessary for now. Let's leave only _open, if other cases appear we will improve it.
@@ -0,0 +26,4 @@xor al, alrepne scasbsub edi, SUFFIX_SIZE + 1I suggest hardcoding to the size of "_open" since other postfixes are not planned yet
@@ -0,0 +48,4 @@mov ebx, fiint 0x40mov eax, SF_TERMINATE_PROCESSI would recommend the mcall macro for system calls:
@@ -67,1 +67,4 @@link_gcc("dgen")tup.include(HELPERDIR .. "/use_fasm.lua")tup.rule({"dgen_open.asm", extra_inputs = {"../../quotkinizer.inc"}}, FASM .. " %f %o " .. tup.getconfig("KPACK_CMD"), "dgen_open")Then something like:
Or perhaps even better, place the tup file in the open_wrap folder, as this is how ports are planned to be removed from the monorep.
WIP: quotes-fixto WIP: Implemented wrapper for @open (fix #336)I just pushed the requested changes and marked them as resolved. Thanks for the tip on the
mcallmacro.@Matou1306
Forgot to remind you to add this there:
@mxlgv Done
@@ -801,6 +801,7 @@ if tup.getconfig('NO_NASM') ~= 'full' then{"kolibrios/games/wolf3d/", VAR_CONTRIB .. "/games/wolf3d/wolf3d"},{"kolibrios/games/wolf3d/", "common/games/wolf3d/*"},{"kolibrios/emul/dgen/dgen", VAR_PROGS .. "/emulator/dgen-sdl-1.33/dgen"},{"kolibrios/emul/dgen/dgen_open", VAR_PROGS .. "/fs/open_wrap/dgen_open"},I believe it should be '/fs/open_wrap/open_wrap' on the right side.
By the way, if we rename the program to dgen_open, blah_open, etc, then maybe it should be called wrap_open, not open_wrap? Just a thought, not a requirement.
That path was depending on the name
fs/wrap_open/Tupfile.luacreated, I updated both.I also renamed
open_wraptowrap_openlike u said, it feels cleaner indeed.@@ -192,3 +192,2 @@ch8=/kolibrios/emul/chip8/chip8md=/kolibrios/emul/dgen/dgengen=/kolibrios/emul/dgen/dgenmd=/kolibrios/emul/dgen/dgen_openWhat does 'md' mean here, markdown? I understand it was like this before your change, I'm just trying to find the logic.
Dgen itself is an ancient Genesis/MegaDrive emulator, so that's what gen and md stand for. The .md extension here just means it's a Sega game image.
(Honestly I had no idea at the start too and I even once tried testing with markdown text lmao)
@@ -0,0 +1,4 @@if tup.getconfig("NO_FASM") ~= "" then return endtup.include("../../use_fasm.lua")tup.rule("open_wrap.asm", FASM .. " %f %o " ..Lines 3 and 4 are a single rule, no need to split it as it is not too long.
@@ -0,0 +1,4 @@if tup.getconfig("NO_FASM") ~= "" then return endtup.include("../../use_fasm.lua")tup.rule("open_wrap.asm", FASM .. " %f %o " ..tup.getconfig("KPACK_CMD"), "dgen_open")Since the final design of the program can be used to call any other program, not just dgen, there is no need to call its binary dgen_open. Let the binary be wrap_open here and then copy it to the dgen directory as dgen_open, to a potential blahblah directory as blahblah_open, etc.
Oh I actually thought that this name was the one that would be added into the dgen directory.
You're right though, I just updated it now.
@Matou1306, please, remove these files: .vscode and test.img
@dunkaist sorry the fasm extension prompted me and automatically added the .vscode files.
Removed thx for catching them alongside the test.img
@@ -0,0 +10,4 @@dd startdd i_enddd m_enddd m_endI think your stack will overwrite the variable 'target_path'. Check a few random programs written in fasm to see how they deal with stack memory.
@@ -0,0 +48,4 @@i_end:params rb 256You define the label 'params' but never use it. Check if the following two lines will do the same as the current lines 51--55:
q_path rb 1
path rb 255
This was there since the start since at first I used to treat params as one single block. When dividing it I just added a virtual block. But like you said there is no difference in the output binary so I'll just keep it simple and do it as you suggested.
Hello, I just want to add a comment that I tested the changes locally yesterday with a real .md game file and they work fine:
I installed an old dgen game (Fix It Felix, Jr.) .md image file and tried opening the file on qemu. It worked just fine. I then tried adding spaces to the path and it also worked, meaning the fix worked and wrap_open handles everything perfectly.
@Matou1306 thanks! It really works.