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

Convert d commands to rzshell #2263

Closed
wants to merge 41 commits into from
Closed

Convert d commands to rzshell #2263

wants to merge 41 commits into from

Conversation

imbillow
Copy link
Contributor

@imbillow imbillow commented Jan 28, 2022

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

...

Test plan

...

Closing issues

Partially addresses #1342

librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved
@XVilka XVilka added shell refactor Refactoring requests labels Jan 29, 2022
Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, it will really help to have this merged!

I think right now the changes are really a lot and hard to review well though. Would it be feasible for you to split this PR into smaller ones? For example, start by converting dd commands, then dmi, etc. etc. Otherwise it becomes hard for me and others to review all changes and it may take a very long time to merge this.

It would be awesome to have this in.

librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved
Comment on lines 330 to 338
- name: dd
summary: >
List file descriptors or Open
/ Open and map that file into the UI
cname: cmd_debug_dd
args:
- name: file
type: RZ_CMD_ARG_TYPE_FILE
optional: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this into ddl (list file descriptors) and dd <file> (Open and map that file)

I think the "UI" part is wrong? What does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even know what it means.

"dd", "", "List file descriptors",
"dd", " <file>", "Open and map that file into the UI",

librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved
librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved
librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved
librz/core/cmd_descs/cmd_debug.yaml Show resolved Hide resolved
summary: List closest symbol to the current address
cname: cmd_debug_dmid
args: []
# - name: dmiv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case ' ': // "dmi "
case '*': // "dmi*"
case 'v': // "dmiv"
case 'j': // "dmij"
case 'q': // "dmiq"
case 'a': // "dmia"
{
const char *libname = NULL, *symname = NULL, *a0;

Because they were processed together in the original, and then it still use RZ_MODE_SIMPLE, RZ_MODE_JSON, but now we seem to use RzOutputMode.
And I haven't completely changed this part dmi.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dmiv seems to be just an alias for dmm

librz/core/cmd_descs/cmd_debug.yaml Outdated Show resolved Hide resolved
librz/core/cmd_descs/cmd_debug.yaml Show resolved Hide resolved
librz/core/cmd_descs/cmd_debug.yaml Show resolved Hide resolved
Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance you could split this PR in smaller ones?

librz/core/cmd/cmd_shell.c Outdated Show resolved Hide resolved
librz/core/cmd_descs/cmd_shell.yaml Outdated Show resolved Hide resolved
librz/core/cmd_descs/cmd_shell.yaml Outdated Show resolved Hide resolved
@imbillow
Copy link
Contributor Author

imbillow commented Mar 16, 2022

Any chance you could split this PR in smaller ones?

I want to fix all the tests first.

@imbillow imbillow mentioned this pull request Mar 16, 2022
5 tasks
@XVilka
Copy link
Member

XVilka commented May 9, 2022

@imbillow shall we close this one then?

@imbillow
Copy link
Contributor Author

imbillow commented May 9, 2022

@imbillow shall we close this one then?

yes

@imbillow imbillow closed this May 9, 2022
@wargio wargio deleted the asan-debug-rzshell branch May 9, 2022 15:34
@imbillow imbillow restored the asan-debug-rzshell branch May 12, 2022 09:34
@imbillow imbillow deleted the asan-debug-rzshell branch May 12, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring requests RzCore shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants