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

cyanpencil · 2018-07-30T15:35:26Z

This should close #10857

Command syntax for ag* commands slightly changes:

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.
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().
Updated ag? help accordingly to this changes. Sent a pr Updated "Graph commands" section of analysis radare2-book#139 to r2book too.
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 · 2018-07-31T03:27:05Z

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

ret2libc · 2018-07-31T14:17:47Z

libr/core/cconfig.c

@@ -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) {


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?

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 · 2018-07-31T17:53:04Z

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 · 2018-07-31T17:55:11Z

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

radare · 2018-08-01T00:24:40Z

libr/core/cmd_anal.c

@@ -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);


why drop the use of specifying the offset as argument?

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.

radare · 2018-08-01T00:43:00Z

libr/core/cmd_anal.c

@@ -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");


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

radare · 2018-08-01T00:43:12Z

libr/core/cmd_anal.c

 			if (cmd && *cmd) {
+				if (*(input + 1)) {


just do input[1]

radare · 2018-08-01T01:03:42Z

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 · 2018-08-01T01:22:09Z

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 · 2018-08-01T01:22:27Z

Using w as write not as web

cyanpencil · 2018-08-01T09:11:29Z

@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:

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 ag command hasn't feature to export graph to given file #10857 for further details
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.

Refactor: Move r_core_graph_cmd from cconfig.c to cmd_anal.c and make it static Refactor: moved getViewerPath() to cmd_anal.c too

Refactor: solve merge conflict in cconfig.c Refactor: other small refactorings in cmd_anal.c

radare · 2018-08-01T10:04:19Z

Replying inline

On 1 Aug 2018, at 11:11, Luca Di Bartolomeo ***@***.***> wrote: @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: 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

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

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.

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

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?

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.

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

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 We can also create a temporary file and move it atomically when ready

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.

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

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cyanpencil · 2018-08-01T14:50:43Z

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 · 2018-08-01T15:15:31Z

@pelijah what do you think?

pelijah · 2018-08-01T17:39:57Z

libr/core/cmd_anal.c

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


Where are my changes? :(

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

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

pelijah · 2018-08-01T17:44:44Z

@Maijin About moving temp file? Hmm ok.

XVilka · 2018-08-02T13:27:05Z

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

cyanpencil · 2018-08-02T14:08:10Z

@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:

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
Use a temporary file like we've been doing since now (a.gif) and simply move it where the user wants.
Use command redirection, e.g. agw @ offset > filename.

XVilka · 2018-08-03T03:52:59Z

@cyanpencil

Kill the argument, we will keep only @ usage. It is mentioned in the book in many places, should be sufficient.
Might be an option
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 · 2018-08-04T11:35:58Z

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

cyanpencil · 2018-08-04T18:22:00Z

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 · 2018-08-04T19:36:30Z

@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 · 2018-08-04T22:01:49Z

All my wishes has been explained already in this issue. Didnt checked last prs so i dont know whats missing or not right now. Yeah feel free to submit the comments in a separate pr

…

On 4 Aug 2018, at 21:36, Anton Kochkov ***@***.***> wrote: @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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cyanpencil mentioned this pull request Jul 30, 2018

Updated "Graph commands" section of analysis radareorg/radare2-book#139

Merged

Maijin mentioned this pull request Jul 30, 2018

ag command hasn't feature to export graph to given file #10857

Closed

XVilka added this to the 2.8.0 milestone Jul 31, 2018

ret2libc reviewed Jul 31, 2018

View reviewed changes

radare reviewed Aug 1, 2018

View reviewed changes

libr/core/cmd_anal.c Outdated

if (cmd && *cmd) {

if (*(input + 1)) {

Copy link

Collaborator

radare Aug 1, 2018

Choose a reason for hiding this comment

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

just do input[1]

cyanpencil added 7 commits August 1, 2018 11:31

Colorize comments like ';arg1' as var type comments

d1ec2dc

Choose path for saving dot graph

44d3770

Fixed ag? help

ab2e3d6

Remove addr argument from most of ag* commands

be22d20

Fix: gcc warnings in r_core_graph_cmd + use vartype comments in cmd_type

b37fd7a

Refactor: Move r_core_graph_cmd from cconfig.c to cmd_anal.c and make it static Refactor: moved getViewerPath() to cmd_anal.c too

Refactor: rename r_core_graph_cmd to graph_cmd

280184c

Refactor: many small refactors of ag* commands in cmd_anal.c

106ae2e

Refactor: solve merge conflict in cconfig.c Refactor: other small refactorings in cmd_anal.c

cyanpencil force-pushed the colorize-arg-cmts branch from e5cd5b1 to 106ae2e Compare August 1, 2018 09:56

pelijah reviewed Aug 1, 2018

View reviewed changes

Fix: wrong merge in cconfig.c and updated ag? help

5ac4f6b

XVilka merged commit c076e12 into radareorg:master Aug 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cyanpencil commented Jul 30, 2018

XVilka commented Jul 31, 2018

ret2libc Jul 31, 2018

cyanpencil Jul 31, 2018

cyanpencil commented Jul 31, 2018

cyanpencil commented Jul 31, 2018

radare Aug 1, 2018

ret2libc Aug 2, 2018

radare Aug 1, 2018

radare Aug 1, 2018

radare commented Aug 1, 2018

radare commented Aug 1, 2018

radare commented Aug 1, 2018

cyanpencil commented Aug 1, 2018

radare commented Aug 1, 2018 via email

cyanpencil commented Aug 1, 2018

Maijin commented Aug 1, 2018

pelijah Aug 1, 2018

cyanpencil Aug 1, 2018

cyanpencil Aug 1, 2018

pelijah commented Aug 1, 2018

XVilka commented Aug 2, 2018

cyanpencil commented Aug 2, 2018

XVilka commented Aug 3, 2018

radare commented Aug 4, 2018

cyanpencil commented Aug 4, 2018

XVilka commented Aug 4, 2018

radare commented Aug 4, 2018 via email

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

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

Conversation

cyanpencil commented Jul 30, 2018

XVilka commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanpencil commented Jul 31, 2018

cyanpencil commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radare commented Aug 1, 2018

radare commented Aug 1, 2018

radare commented Aug 1, 2018

cyanpencil commented Aug 1, 2018

radare commented Aug 1, 2018 via email

cyanpencil commented Aug 1, 2018

Maijin commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelijah commented Aug 1, 2018

XVilka commented Aug 2, 2018

cyanpencil commented Aug 2, 2018

XVilka commented Aug 3, 2018

radare commented Aug 4, 2018

cyanpencil commented Aug 4, 2018

XVilka commented Aug 4, 2018

radare commented Aug 4, 2018 via email