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.

Sign up for GitHub

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

Jump to bottom

Port search commands (/) to newshell #2212

Closed
wants to merge 34 commits into from
Closed

Port search commands (/) to newshell #2212

wants to merge 34 commits into from

Conversation

DMaroo
Copy link
Member

@DMaroo DMaroo commented Jan 15, 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

This PR ports the search commands (/) to Rizin newshell.

Test plan

Once the porting is done, the build and CI should be green. Maybe I'll add tests for any new features that get added during the porting.

Closing issues

Closes #1586.
Tracked by #1342.

@DMaroo DMaroo force-pushed the port-search branch 2 times, most recently from 637b549 to fdf7318 Compare January 15, 2022 09:01
@XVilka XVilka mentioned this pull request Jan 15, 2022
Open
23 tasks
XVilka
XVilka requested changes Jan 31, 2022
View reviewed changes
@DMaroo DMaroo force-pushed the port-search branch 2 times, most recently from 602e077 to 67220ef Compare February 8, 2022 20:01
@DMaroo DMaroo force-pushed the port-search branch 2 times, most recently from f5675f1 to 908f2bf Compare February 25, 2022 12:05
ret2libc
ret2libc reviewed Feb 25, 2022
View reviewed changes
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.

I think the RzSearchParameters split make sense, but I'd have a new _new API that accepts a RzSearchParameters structure to initialize the search. I think starting writing unit tests will definitely help to find out what is the best way to deal with this.

I'd see something like the following:

RzSearchParams params = {
.mode = RZ_SEARCH_STRING,
.string_min = 3,
.string_max = 10,
};
RzSearch *s = rz_search_new(&params);

note that some of the parameters may be meaningful only for a specific "search mode". In such cases it may make sense to have RzSearchParameters composed of a union for the data, selected by the search_mode field.

librz/core/cmd/cmd_print.c
@@ -844,6 +844,36 @@ static const ut32 colormap[256] = {
0xffffff,
};

static void incAlphaBuffer(ut8 *buf, int bufsz) {
Copy link
Member

Choose a reason for hiding this comment

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

what are these?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are some functions that I needed to keep such that the build passes, I will refactor them in the end.

@github-actions github-actions bot added the FLIRT label Feb 26, 2022
ret2libc
ret2libc reviewed Feb 26, 2022
View reviewed changes
@DMaroo DMaroo force-pushed the port-search branch 2 times, most recently from 8681936 to 7b7744a Compare March 12, 2022 10:05
DMaroo added 18 commits December 11, 2022 14:31
@DMaroo
Add RzSearchParams struct
91910fa 
  * This will allow for a generic search function, since all search
    parameters can be represented using the `RzSearchParams` struct
  * Also, the struct contains search parameters, so any search function
    (whose job is to "search") would never need to modify any of these
    params. This allows for some sort of contraint/invariance and will
    help in making sure that the search functions which get implemented
    are correct.
  * Also added `search.minlength` and `search.maxlength` in `cconfig.c`
@DMaroo
Add new and free functions for RzSearchParams
bb7a252 
  * Rename `RzSearchParameters` to `RzSearchParams`
  * Rename struct member names
  * Add `rz_search_params_new`, `rz_core_search_params_new` and
    `rz_search_params_free`
  * Rename `rz_search_set_mode` to `rz_search_params_set_mode`
@DMaroo
Remove RzCore from rz_search.h
10dc93e
@DMaroo
Fix the logic in rz_str_extract_printable
f149dc9
@DMaroo
Use RZ_LOG_INFO instead of rz_cons_printf
9bb1543
@DMaroo
Change details for cx
ee6c84a
@DMaroo
Remove rz_core_search_params_new; Use rz_search_params_new instead
a4e93d9
@DMaroo
Add unit test for rz_str_extract_printable
a00b985
@DMaroo
Replace rz_search_set_distance and rz_search_kw_add with their `R…
8d2c0d0 
…zSearchParams` counterpart
@DMaroo
Initialize search parameters in rz_core_init
92c17e9
@DMaroo
Add ownership annotation for rz_search_new
fc9d1a0
@DMaroo
Format according to clang-format
b093328
@DMaroo
Replace rz_search_string_prepare_backward with RzSearchParams var…
0146789 
…iant
@DMaroo
Use RzSearchParam callback in rz_search_find if it exists
027f6e8
@DMaroo
Removed unused duplicate function rz_search_update_i
0318a35
@DMaroo
Replace eprintf; Fix code typos
6c2c927
@DMaroo
Remove unused functions rz_search_find, listcb
3735881
@DMaroo
Fix merge conflict remnants
155057a
@XVilka
Copy link
Member

XVilka commented Jan 19, 2023

It needs rebase. If you finish some particular command or few of them - probably better to focus on that, so the porting is incremental. Otherwise, we have to postpone this for 0.6.0

@XVilka
Copy link
Member

XVilka commented Apr 6, 2023

@DMaroo do you plan to continue working on this? Otherwise it probably better to close this one and start from scratch.

@DMaroo
Copy link
Member Author

DMaroo commented Apr 6, 2023

@XVilka I have been very low on time lately. So I don't expect any progress on this for a while. Closing it seems reasonable. I'll open a new PR whenever I get done with the porting.

@XVilka XVilka mentioned this pull request Jul 4, 2023
Merged
5 tasks
@DMaroo
Copy link
Member Author

DMaroo commented Oct 27, 2023

Changes have become stale and there are too many conflicts. Closing this PR for now. ETA on this is undecided.

@DMaroo DMaroo closed this Oct 27, 2023
@ret2libc
Copy link
Member

I'm going to work on this as discussed on mattermost. I will start with /R and then expand from there. I will just try to convert the commands for now and leave the RzSearch refactoring to later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ret2libc ret2libc ret2libc left review comments

@XVilka XVilka XVilka requested changes

@thestr4ng3r thestr4ng3r Awaiting requested review from thestr4ng3r thestr4ng3r will be requested when the pull request is marked ready for review thestr4ng3r is a code owner

@wargio wargio Awaiting requested review from wargio wargio will be requested when the pull request is marked ready for review wargio is a code owner

@kazarmy kazarmy Awaiting requested review from kazarmy kazarmy will be requested when the pull request is marked ready for review kazarmy is a code owner

@Rot127 Rot127 Awaiting requested review from Rot127 Rot127 will be requested when the pull request is marked ready for review Rot127 is a code owner

Assignees

@ret2libc ret2libc

Labels
API ESIL rz-find rz-test RzCore RZIL RzSearch RzUtil waiting-for-author Used to mark PRs where more work is needed
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

Port / (search) to rzshell
3 participants
@DMaroo @XVilka @ret2libc