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

Add "save to file" to ag*w commands + colorize comments like ";arg1" #10860

Merged
merged 8 commits into from Aug 4, 2018

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented Jul 30, 2018

This should close #10857

Command syntax for ag* commands slightly changes:

  1. No more [fcn addr] argument; this was useless, now it only uses core->offset. If you want to draw graph of a function with an offset different from the current one, just do agfv @ other_offset. The only exception is agd command, because you must provide the offset of the function to diff against.

  2. Now the w (image) format provides an argument, which is the file path to save the image to. If no path is provided, the image is displayed immediately (like before). Slight refactoring of r_core_graph_cmd().

  3. Updated ag? help accordingly to this changes. Sent a pr radareorg/radare2-book#139 to r2book too.

  4. Comments like ";arg1" will be of the same color of the declaration of arguments, for consistency. I added this pr because it is really just a one line change that I forgot in my last colourize arguments pr.

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 31, 2018

@cyanpencil Travis is red, can you please take care of it?

@XVilka XVilka added this to the 2.8.0 milestone Jul 31, 2018
@@ -2290,29 +2290,30 @@ static char *getViewerPath() {
return NULL;
}

R_API char* r_core_graph_cmd(RCore *core, char *r2_cmd) {
R_API char* r_core_graph_cmd(RCore *core, char *r2_cmd, char *save_path) {

This comment has been minimized.

@ret2libc

ret2libc Jul 31, 2018
Contributor

just a random question not strictly related to this PR, but... why is this function in cconfig.c ? isn't it more appropriate for core/graph.c?

This comment has been minimized.

@cyanpencil

cyanpencil Jul 31, 2018
Author Contributor

Actually you are right... It was here because it was used to initialize the graph.cmd variable, but now it has no more sense to keep it here, I'll move it

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 31, 2018

It was my small change to types comments that made travis angry, now I modified r_core_anal_get_comments to consider vartype comments too, and should be green.
Also made r_core_graph_cmd static and moved to cmd_anal, since it is only used there

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 31, 2018

Wait forgot to rename it, since it is now static it should be renamed from r_core_graph_cmd() to graph_cmd() only

@@ -5961,8 +6010,7 @@ static void cmd_anal_graph(RCore *core, const char *input) {
case 't': { // "agft" - tiny graph
int e = r_config_get_i (core->config, "graph.edges");
r_config_set_i (core->config, "graph.edges", 0);
ut64 off_fcn = (*(input + 2)) ? r_num_math (core->num, input + 2) : core->offset;
RAnalFunction *fcn = r_anal_get_fcn_in (core->anal, off_fcn, 0);
RAnalFunction *fcn = r_anal_get_fcn_in (core->anal, core->offset, 0);

This comment has been minimized.

@radare

radare Aug 1, 2018
Collaborator

why drop the use of specifying the offset as argument?

This comment has been minimized.

@ret2libc

ret2libc Aug 2, 2018
Contributor

I think it makes more sense to have a consistent api that uses @ addr to seek to address, instead of having arguments for the seek.

@@ -5922,8 +5969,12 @@ static void cmd_agraph_print(RCore *core, const char *input) {
if (r_config_get_i (core->config, "graph.web")) {
r_core_cmd0 (core, "=H /graph/");
} else {
char *cmd = r_core_graph_cmd (core, "aggd");
if (cmd && *cmd) {
if (*(input + 1)) {

This comment has been minimized.

@radare

radare Aug 1, 2018
Collaborator

just do input[1]

@radare
Copy link
Collaborator

@radare radare commented Aug 1, 2018

dosnt seems consistent because it only works with w, right?, please fix the conflict.

also the %s in the r_core_cmd scares me a bit because of the possible command injections.

btw, graph.extension looks a bit long. maybe we shuold name it graph.gv.format or imagetype or something like this, because its a graphviz specific var

  • help message is not alphabetically ordered

  • and yeah.. we already have >. having file as argument is usually handy, but must be consistent with the other commands i think.

@radare
Copy link
Collaborator

@radare radare commented Aug 1, 2018

Also ‘w’ is a bit misleading because its not related to web. And depends on dot. So maybe a aubcommand of agd like agdw? That would make more sense to me

@radare
Copy link
Collaborator

@radare radare commented Aug 1, 2018

Using w as write not as web

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Aug 1, 2018

@radare Thanks for the review! I'll try to address all your comments here:

why drop the use of specifying the offset as argument?

It was suggested to me by @Maijin, for two main reasons:

  1. Cutter needed a way to use a "save to" feature, where it is be possible to choose a filename. Before, the argument of agw would specify the offset of the function to graph; now it takes as argument the filepath to save the graph image to. See issue #10857 for further details
  2. It honors the r2 philosophy of cmd @ offset, which is more consistent with many other r2 commands. To plot a different function than the current offset, the user can still do agw filename @ offset.

in https://github.com/radare/radare2/pull/10860/files#diff-9a7bcc9d9f9b6439c4d8b9301f1d46ccR5947 honor graph.font eval var

Good point, will do it

just do input[1]

Sure, will do that too

dosnt seems consistent because it only works with w, right?

Yes, it only works with w. The other ag formats ignore arguments. I know it's not very clear, but I tried my best to reflect that in the ag? help. I hope it is understandable.

also the %s in the r_core_cmd scares me a bit because of the possible command injections.

Hmmm, that %s string is built from user input so shouldn't it be safe? A way to make it safer doesn't come to my mind. Do you have any suggestions on how could I improve it?

btw, graph.extension looks a bit long. maybe we shuold name it graph.gv.format or imagetype or something like this, because its a graphviz specific var

Yes you are right, graph.gv.format is a better name, will change it

help message is not alphabetically ordered

Ok, fixing it

and yeah.. we already have >. having file as argument is usually handy, but must be consistent with the other commands i think.

Imho it's a bit different, look at the brief discussion in #10857, redirecting the output of a command to a file and saving an image with a given filename are slightly differnet things... I realize that there is a consistency problem here, but no better syntax has come to my mind on how to support this cutter feature. I thought about an additional config var, but that would be actually worse than this

Also ‘w’ is a bit misleading because its not related to web. And depends on dot. So maybe a aubcommand of agd like agdw? That would make more sense to me

Unfortunately it's not that simple: this does not relate only to agw command, but to all ag* commands ending in w (that means their output is in image format). See ag? help for the list of formats and commands. agd is the diff graph, agdd is the diff graph in dot format (plaintext), agdw is the diff graph in image format (with graphviz) and so on. In this case agw is just an alias for agfw, which is the basic block graph of a function

Using w as write not as web

Good point, since web is not supported anymore, I'll change the ag? help to use write instead of web.

@cyanpencil cyanpencil force-pushed the cyanpencil:colorize-arg-cmts branch from e5cd5b1 to 106ae2e Aug 1, 2018
@radare
Copy link
Collaborator

@radare radare commented Aug 1, 2018

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Aug 1, 2018

Ok but better use a subcommand like i said and make that behaviour consistent with all the other commands

We could add something like wf, and use commands like agwf [file]. Or maybe even W, so we could get agW [file] @Maijin what do you think about this?

Yes makes sense but help message doesnt needs to reflect that @.. stuff on every subcommand

Ok, will update the help message

Printing a png to stdout in binary form is usually not useful and undesired but its the unix way and how dot works by default

Let me understand you better...
You are proposing the following behaviour:
if agw > file is called, than print the image to stdout (and so write to file)
if agw is called without stdout redirection, than display the image like we always did until now.
Is that right?

We can also create a temporary file and move it atomically when ready

Well, this is what is done for immediately displaying the graph image (agw without filename). We just create a.gif (or a.svg, a.png etc...) and then use the viewer to open it. @Maijin could this be an option, creating only a.gif and the calling mv a.gif /path/to/filename/ from cutter?

Wat? The json shoukd be enough for the web to work (or render it as an image)

Yes, sorry, I was just referring to changing the text on the help message

We may probably try want to create thar file using apis instead of putting that unfiltered text inside a system() line. Maybe check for some chars and failfast if any? I think is better to tweak rcons tee var to make that file be created by apis and fail before running the command if cant be created.

I did not understand what you meant here :( Are you saying that we should try to create the provided filename and print a few characters to it to check if it erros before actually drawing the image graph?

@Maijin
Copy link
Contributor

@Maijin Maijin commented Aug 1, 2018

@pelijah what do you think?

static char *getViewerPath() {
int i;
const char *viewers[] = {
"open",

This comment has been minimized.

@pelijah

pelijah Aug 1, 2018
Contributor

Where are my changes? :(

This comment has been minimized.

@cyanpencil

cyanpencil Aug 1, 2018
Author Contributor

Wait, maybe I fucked up the merge of the conflicts, let me check

This comment has been minimized.

@cyanpencil

cyanpencil Aug 1, 2018
Author Contributor

I copied the old getViewerPath and left the new one in cconfig.c :(
Sorry, my bad, will fix it

@pelijah
Copy link
Contributor

@pelijah pelijah commented Aug 1, 2018

@Maijin About moving temp file? Hmm ok.

@XVilka
Copy link
Contributor

@XVilka XVilka commented Aug 2, 2018

So, what about this one? Review became a very long one...

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Aug 2, 2018

@XVilka Yes, better give a TL;DR

We need to decide what's the best syntax to decide how can the user / cutter decide where to save the agw output. Here are the avialable options proposed until now:

  1. Use the agw argument. So that would mean agw filename @ offset. This is what this pr implements. @radare said (correctly) that this is a bit inconsistent with the other ag commands which do not require any argument

  2. Use a temporary file like we've been doing since now (a.gif) and simply move it where the user wants.

  3. Use command redirection, e.g. agw @ offset > filename.

@XVilka
Copy link
Contributor

@XVilka XVilka commented Aug 3, 2018

@cyanpencil

  1. Kill the argument, we will keep only @ usage. It is mentioned in the book in many places, should be sufficient.
  2. Might be an option
  3. Good idea.

Please address the comments and we'll merge. Any further additions should be done with the next PR. I think it is in the good shape already.

@radare
Copy link
Collaborator

@radare radare commented Aug 4, 2018

ping. release is in 2 days. lets try to make this into 2.8

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Aug 4, 2018

I addressed the missing things.
Unfortunately I wasn't able to implement the agw > redirection, it seems like radare has some trouble with nested redirections, and fixing those was out of scope of this pr, so we have to stick with agw filename or the temporary file.
For now this pr implements the agw filename @ offset, if you guys prefer the temporary file behaviour I'll quickly change it, should be a trivial change

@XVilka XVilka merged commit c076e12 into radareorg:master Aug 4, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@XVilka
Copy link
Contributor

@XVilka XVilka commented Aug 4, 2018

@radare if you think something need to be changed - ask @cyanpencil make a new PR. Merged this one so it will not start to rot.

@radare
Copy link
Collaborator

@radare radare commented Aug 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants