Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4889 closed defect (implemented)

Cleanup: use smartlist_add_asprintf() rather than tor_asprintf+smartlist_add()

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We have a smartlist_add_asprintf() alias; let's use it!

circuit_describe_status_for_controller() needs help here, but probably other cases do too.

We should also grep around a little for more places we should be using tor_asprintf in the first place.

Child Tickets

Change History (9)

comment:1 in reply to:  description Changed 8 years ago by rransom

Replying to nickm:

We have a smartlist_add_asprintf() alias; let's use it!

It's currently called smartlist_asprintf_add. smartlist_add_asprintf sounds better, though.

comment:2 Changed 8 years ago by nickm

Status: newneeds_review

Renamed the function, and did a big pile of cleanups in branch "bug4889".

It's a little long, but the changes are all local and straightforward. The biggest thing to watch out for would be some case where we did

  len = very big thing
  x = tor_malloc(len);
  tor_snprintf(x,len,"foo%s%s...",...);
  append more stuff to x;

and I accidentally replaced it with

  tor_asprintf(&x, "foo%s%s...",...);
  append morestuff to x;

But I think I avoided all those cases.

comment:3 in reply to:  2 Changed 8 years ago by rransom

Replying to nickm:

Renamed the function, and did a big pile of cleanups in branch "bug4889".

Looks good!

It's a little long, but the changes are all local and straightforward. The biggest thing to watch out for would be some case where we did

  len = very big thing
  x = tor_malloc(len);
  tor_snprintf(x,len,"foo%s%s...",...);
  append more stuff to x;

and I accidentally replaced it with

  tor_asprintf(&x, "foo%s%s...",...);
  append morestuff to x;

But I think I avoided all those cases.

I didn't see any of those. The scariest part of this branch to me is the part of commit 8a7f46e1fe5e867cd0f8c7d24f4a7b66ede1a94a in policy_summarize (src/or/policies.c), which needs careful re-review.

comment:4 Changed 8 years ago by nickm

On re-review, that part of policy_summarize looks okay to me. It's killing the size precomputation for final_size, which is only used when allocating result, which is now allocated by tor_asprintf().

comment:5 Changed 8 years ago by nickm

I pushed a couple of squash commits to branch bug4889. They don't fix bugs, but they try a little harder to do defensive programming on this. Comments invited.

comment:6 Changed 8 years ago by Sebastian

The original branch has two commits with the exact same description, so the autosquash will not do what you want. Be careful when rebasing this. It'd probably make sense to squash the two commits with the same description together, or give them a different first line of description - any time I read a commit list where two entries are the same next by each other, I'm going to assume something went weird when the commits were made.

Otherwise, I couldn't find a problem with either the original branch nor the fixups you introduced here.

If you want, I have a branch with the history of your branch somewhat cleaned up (imo) in my bug4889 branch, so you don't have to wrangle with git yourself. The diff to your branch is this:

index 6138201..ef96c06 100644
--- a/changes/clean_asprintf
+++ b/changes/clean_asprintf
@@ -2,4 +2,6 @@
     - Use the smartlist_add_asprintf alias more consistently
       throughout the codebase.
     - Convert more instances of tor_snprintf+tor_strdup into
-      tor_asprintf.
\ No newline at end of file
+      tor_asprintf.
+    - Convert more instances of tor_snprintf+tor_malloc into
+      tor_asprintf.

but also note the changes in commit message to make them distinct.

comment:7 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Couldn't find your branch, so I re-did my own and merged it. Thanks!

comment:8 Changed 7 years ago by nickm

Keywords: tor-client added

comment:9 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.