KolibriOS/kolibrios
15
24
Fork 61
Code Issues 132 Pull Requests 31 Actions Projects 1 Releases Wiki Activity

ftpd: Add RMD command #353

Merged
Burer merged 4 commits from igorsh/kolibrios:ftpd-add-rmd-command into main 2026-05-19 18:40:25 +00:00
Conversation 10 Commits 4 Files Changed 1
+18 -10
igorsh commented 2026-03-05 19:06:33 +00:00
Contributor
Copy Link
Copy Source

Added RMD command

Added RMD command
igorsh added 1 commit 2026-03-05 19:06:34 +00:00
ftpd: Add RMD command
Build system / Check kernel codestyle (pull_request) Successful in 26s
Details
Build system / Build (pull_request) Successful in 17m0s
Details
3d73e80d39
mxlgv added the Category/ApplicationsFASM
Kind
Enhancement
Priority
Low
 labels 2026-03-11 10:31:42 +00:00
mxlgv requested review from Burer 2026-03-14 16:52:55 +00:00
mxlgv requested review from Doczom 2026-03-14 16:52:57 +00:00
mxlgv requested review from dunkaist 2026-03-14 16:53:00 +00:00
mxlgv requested review from mxlgv 2026-03-14 16:53:03 +00:00
dunkaist reviewed 2026-03-14 19:42:45 +00:00
programs/network/ftpd/commands.inc Outdated
@@ -462,2 +461,3 @@
; "DELE" & "RMD"
;
; Delete a file from the server.
; Delete a file/folder from the server.
dunkaist commented 2026-03-14 19:42:45 +00:00
Owner
Copy Link
Copy Source

I believe the letter D in RMD means not 'folder'

I believe the letter D in RMD means not 'folder'
dunkaist marked this conversation as resolved
dunkaist reviewed 2026-03-14 19:44:01 +00:00
programs/network/ftpd/commands.inc Outdated
@@ -472,2 +472,2 @@
; Create path
cmp ecx, 1024 + 5
mov edx, 4
cmp byte[esi], 0x52 ; R
dunkaist commented 2026-03-14 19:44:01 +00:00
Owner
Copy Link
Copy Source

Fasm understands just 'R'. Same with 'r' below

Fasm understands just 'R'. Same with 'r' below
dunkaist marked this conversation as resolved
dunkaist reviewed 2026-03-14 20:25:22 +00:00
programs/network/ftpd/commands.inc Outdated
@@ -474,0 +475,4 @@
cmp byte[esi], 0x72 ; r
je @f
mov edx, 5
dunkaist commented 2026-03-14 20:25:21 +00:00
Owner
Copy Link
Copy Source

Is this 5 strlen("DELE ")? Please, describe the logic around these lines in a comment

Is this 5 strlen("DELE ")? Please, describe the logic around these lines in a comment
👀 1
dunkaist commented 2026-03-18 19:51:52 +00:00
Owner
Copy Link
Copy Source

Don't forget to describe this logic with a comment, please. Thank you

Don't forget to describe this logic with a comment, please. Thank you
👍 1
dunkaist commented 2026-03-27 09:18:45 +00:00
Owner
Copy Link
Copy Source

Don't forget to describe this logic with a comment, please. Thank you

Any update, @igorsh?

> Don't forget to describe this logic with a comment, please. Thank you Any update, @igorsh?
igorsh commented 2026-04-18 03:58:15 +00:00
Author
Contributor
Copy Link
Copy Source

Кладем в edx длину команды - 4 и если команда rmd то прыгаем дальше, в противном случае кладем в edx 5, это значит команда DELE

Кладем в edx длину команды - 4 и если команда rmd то прыгаем дальше, в противном случае кладем в edx 5, это значит команда DELE
dunkaist commented 2026-05-13 20:15:53 +00:00
Owner
Copy Link
Copy Source

@igorsh, please, add a code comment describing this logic. If you can't get rid of those magic numbers, there should be a code comment describing their meaning and use. Thank you

@igorsh, please, add a code comment describing this logic. If you can't get rid of those magic numbers, there should be a code comment describing their meaning and use. Thank you
Burer marked this conversation as resolved
igorsh added 2 commits 2026-03-17 18:11:24 +00:00
ftpd: Change comment 22b07ef5a5
ftpd: Use literal R & r
Build system / Check kernel codestyle (pull_request) Successful in 1m52s
Details
Build system / Build (pull_request) Successful in 11m28s
Details
a5b1c32f04
dunkaist requested changes 2026-03-18 19:53:03 +00:00
Dismissed
dunkaist left a comment
Owner
Copy Link
Copy Source

One of my comments is still not addressed: #353 (comment)

One of my comments is still not addressed: https://git.kolibrios.org/KolibriOS/kolibrios/pulls/353#issuecomment-5037
igorsh marked the pull request as work in progress 2026-03-21 06:27:15 +00:00
igorsh marked the pull request as ready for review 2026-05-13 16:48:27 +00:00
Burer commented 2026-05-18 12:19:28 +00:00
Owner
Copy Link
Copy Source

@dunkaist @igorsh

Does this version looks okay?
If so, I will push it into PR branch so we can finally merge this PR.

diff --git a/programs/network/ftpd/commands.inc b/programs/network/ftpd/commands.inc
index b508acab0..6cdc99774 100644
--- a/programs/network/ftpd/commands.inc
+++ b/programs/network/ftpd/commands.inc
@@ -102,7 +102,7 @@ commands:               ; all commands must be in uppercase
 ;        dd 'REIN', login_first, login_first, login_first, cmd_REIN
 ;        dd 'REST', login_first, login_first, login_first, cmd_REST
         dd 'RETR', login_first, login_first, login_first, cmdRETR
-;        dd 'RMD', login_first, login_first, login_first, cmd_RMD
+        dd 'RMD', login_first, login_first, login_first, cmdDELE
 ;        dd 'RNFR', login_first, login_first, login_first, cmd_RNFR
 ;        dd 'RNTO', login_first, login_first, login_first, cmd_RNTO
 ;        dd 'SITE', login_first, login_first, login_first, cmd_SITE
@@ -458,9 +458,9 @@ cmdCWD:
         ret

 ;------------------------------------------------
-; "DELE"
+; "DELE" & "RMD"
 ;
-; Delete a file from the server.
+; Delete a file/directory from the server.
 ;
 ;------------------------------------------------
 align 4
@@ -469,16 +469,24 @@ cmdDELE:
         test    [ebp + thread_data.permissions], PERMISSION_DELETE
         jz      permission_denied

-        ; Create path
-        cmp     ecx, 1024 + 5
-        jae     .err
-
-        sub     ecx, 5
-        jb      .err
+        ; "DELE <path>" and "RMD <path>" share this handler. Skip the
+        ; command + its trailing space to reach <path>: "RMD " = 4,
+        ; "DELE " = 5 bytes. RMD is the only 'R' command routed here.
+        mov     edx, 4                  ; strlen("RMD ")
+        cmp     byte[esi], 'R'
+        je      @f
+        cmp     byte[esi], 'r'
+        je      @f
+        mov     edx, 5                  ; strlen("DELE ")
+  @@:
+        sub     ecx, edx                ; ecx = <path> length
+        jb      .err                    ; command line shorter than prefix
+        cmp     ecx, 1024               ; <path> must fit the path buffer
+        jae     .err

         call    create_path
         dec     edi
-        lea     esi, [ebp + thread_data.buffer + 5]
+        lea     esi, [ebp + thread_data.buffer + edx]
         mov     ecx, 1024
         cmp     byte [esi], '/'
         jne     .loop
@@ -512,8 +512,8 @@ cmdDELE:
         test    eax, eax
         jnz     .err

         sendFTP "250 Command successful"
         ret
 .err:
-        sendFTP "550 No such file"
+        sendFTP "550 Requested action not taken"
         ret
@dunkaist @igorsh Does this version looks okay? If so, I will push it into PR branch so we can finally merge this PR. ```diff diff --git a/programs/network/ftpd/commands.inc b/programs/network/ftpd/commands.inc index b508acab0..6cdc99774 100644 --- a/programs/network/ftpd/commands.inc +++ b/programs/network/ftpd/commands.inc @@ -102,7 +102,7 @@ commands: ; all commands must be in uppercase ; dd 'REIN', login_first, login_first, login_first, cmd_REIN ; dd 'REST', login_first, login_first, login_first, cmd_REST dd 'RETR', login_first, login_first, login_first, cmdRETR -; dd 'RMD', login_first, login_first, login_first, cmd_RMD + dd 'RMD', login_first, login_first, login_first, cmdDELE ; dd 'RNFR', login_first, login_first, login_first, cmd_RNFR ; dd 'RNTO', login_first, login_first, login_first, cmd_RNTO ; dd 'SITE', login_first, login_first, login_first, cmd_SITE @@ -458,9 +458,9 @@ cmdCWD: ret ;------------------------------------------------ -; "DELE" +; "DELE" & "RMD" ; -; Delete a file from the server. +; Delete a file/directory from the server. ; ;------------------------------------------------ align 4 @@ -469,16 +469,24 @@ cmdDELE: test [ebp + thread_data.permissions], PERMISSION_DELETE jz permission_denied - ; Create path - cmp ecx, 1024 + 5 - jae .err - - sub ecx, 5 - jb .err + ; "DELE <path>" and "RMD <path>" share this handler. Skip the + ; command + its trailing space to reach <path>: "RMD " = 4, + ; "DELE " = 5 bytes. RMD is the only 'R' command routed here. + mov edx, 4 ; strlen("RMD ") + cmp byte[esi], 'R' + je @f + cmp byte[esi], 'r' + je @f + mov edx, 5 ; strlen("DELE ") + @@: + sub ecx, edx ; ecx = <path> length + jb .err ; command line shorter than prefix + cmp ecx, 1024 ; <path> must fit the path buffer + jae .err call create_path dec edi - lea esi, [ebp + thread_data.buffer + 5] + lea esi, [ebp + thread_data.buffer + edx] mov ecx, 1024 cmp byte [esi], '/' jne .loop @@ -512,8 +512,8 @@ cmdDELE: test eax, eax jnz .err sendFTP "250 Command successful" ret .err: - sendFTP "550 No such file" + sendFTP "550 Requested action not taken" ret ```
dunkaist commented 2026-05-18 15:16:27 +00:00
Owner
Copy Link
Copy Source

@Burer, looks good to me, thank you!

@Burer, looks good to me, thank you!
❤️ 1
igorsh commented 2026-05-18 19:55:34 +00:00
Author
Contributor
Copy Link
Copy Source

@Burer looks good

@Burer looks good
Burer added 1 commit 2026-05-19 08:13:52 +00:00
ftpd: address review for RMD command
Build system / Check kernel codestyle (pull_request) Successful in 27s
Details
Build system / Build (pull_request) Successful in 9m38s
Details
16f3a6ae9a 
- document the shared DELE/RMD handler and the prefix-length logic
  (annotate strlen("RMD ")/"DELE " instead of bare magic numbers)
- de-obfuscate the path bounds check (drop the add/sub 1024 round-trip)
- use a command-neutral 550 reply ("Requested action not taken")
Burer removed review request for mxlgv 2026-05-19 09:12:24 +00:00
Burer requested review from hidnplayr 2026-05-19 09:12:29 +00:00
Burer approved these changes 2026-05-19 09:23:21 +00:00
dunkaist approved these changes 2026-05-19 09:26:24 +00:00
Doczom approved these changes 2026-05-19 10:51:45 +00:00
Burer merged commit 68f0454682 into main 2026-05-19 18:40:25 +00:00
Burer deleted branch ftpd-add-rmd-command 2026-05-19 18:40:25 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
hidnplayr
Burer
dunkaist
Doczom
Labels
Clear labels
C
Category/Applications
Category/Drivers
Category/General
Category/Kernel
Category/Libraries
Eolite
FASM
FS
GSoC
This issue in GSoC program
 HardwareTested
HLL
Influence/Settings
Influence/Text/TYPO
IRCC
Kernel
Kind
Breaking
Breaking change that won't be backward compatible
Kind
Bug
Something is not working
Kind
Build
Kind
Documentation
Documentation changes
Kind
Enhancement
Improve existing functionality
Kind
Feature
New functionality
Kind
Security
This is security issue
Kind
Testing
Issue or pull request related to testing
Pay for the code
Paid task
PR
Conflicts
PR conflicts with main
PR
Dependent
This PR is dependent on another PR
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
PR
Ready to merge
Pull request is ready for merge
PR
Request changes
Changes requested in pull request
PR
Review required
Reviewed
Confirmed
Issue has been confirmed
Reviewed
Duplicate
This issue or pull request already exists
Reviewed
Invalid
Invalid issue
Reviewed
Won't Fix
This issue won't be fixed
Status
Abandoned
Somebody has started to work on this but abandoned work
Status
Blocked
Something is blocking this issue or pull request
Status
Need More Info
Feedback is required to reproduce issue or to continue work
No Label Category/Applications FASM
Kind
Enhancement
Priority
Low
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ace-dent (Andrew) Burer Doczom (Mikhail Frolov) dunkaist (Ivan B) Egor00f (Егор) hidnplayr IgorA Leency (Kiril Lipatov) mxlgv (Max Logaev) ramenu rgimad sdongles (Serhii Sakhno) Sweetbread (Gleb Zaharov)
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KolibriOS/kolibrios#353
Delete Branch "igorsh/kolibrios:ftpd-add-rmd-command"

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?