Opened 8 years ago

Closed 6 years ago

#3726 closed defect (wontfix)

obfsproxy needs contrib/checkSpace.pl

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: #3591 Points:
Reviewer: Sponsor:

Description

When the code "stabilizes" a bit, we should bring checkSpace.pl to the obfsproxy world to enforce some code style sanity.

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by asn

Status: newneeds_review

Please see branch bug3726 in https://git.gitorious.org/obfsproxy/obfsproxy.git.

I think it's a good time to introduce checkSpace.pl in obfsproxy. The code is stable (till #4685 stuff appear), and the only thing remaining to get merged is #3729.

BTW, 5cc436a6 in branch bug3726 is a bit crude. I would, instead, prefer to remove the cast to unsigned long (and also remove it in all the other similar cases in obfs2.c and network.c). How do we feel about C99 and its 'z' format string length specifier ("%zu" for size_t, for example)?

comment:2 Changed 8 years ago by nickm

Actually, obfsproxy is young enough and has few enough pending patchs that I'd be comfortable with a less hackish, more global soltion. How would you feel about doing an appropriate uncrustify configuration?

Windows doesn't have %zu, IIRC.

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

Replying to nickm:

Actually, obfsproxy is young enough and has few enough pending patchs that I'd be comfortable with a less hackish, more global soltion. How would you feel about doing an appropriate uncrustify configuration?

Oh, I've never heard of uncrustify. I'll check it out.

Windows doesn't have %zu, IIRC.

<ticket hijack>

Hm, what about creating a SIZE_T_FORMAT (à la tor's U64_FORMAT), which becomes "Iu" in MinGW [0], and "zu" in the other compilers? Do you think this will cause too much breakage to exotic/old compilers? If yes, I guess leaving it as is, is more than fine.

</ticket hijack>

[0]: http://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

comment:4 Changed 8 years ago by asn

I just finished a 6 hours session of looking at uncrustify...

You can see my custom config and the results of uncrustify, in branch bug3726_uncrustify_test in https://git.gitorious.org/obfsproxy/obfsproxy.git:
https://gitorious.org/obfsproxy/obfsproxy/commits/bug3726_uncrustify_test
https://gitorious.org/obfsproxy/obfsproxy/commit/dae09f28999d5ce29dc6a4137ad1e2379e8cc5e8

All in all, it seems useful and very versatile, but it still has its issues:

Issues:

  • Sometimes it splits function arguments very stupidly:
     struct evutil_addrinfo *resolve_address_port(const char *address,
    -                                             int nodns, int passive,
    +                                             int nodns,
    +                                             int passive,
                                                  const char *default_port);
    

or much worse:

 void
-smartlist_uniq(smartlist_t *sl,
-               int (*compare)(const void **a, const void **b),
-               void (*free_fn)(void *a))
+smartlist_uniq(smartlist_t *sl,int (*compare)(const void **a,
+                                              const void **b),void (*free_fn)(
+                 void *a))

  • There is a boolean option called indent_func_call_param for which:
    True:  indent continued function call parameters one indent level
    False: align parameters under the open paren
    

So if it's true, this ugly thing happens:

       memmove(sl->list + idx + 1, sl->list + idx,
 -              sizeof(void*)*(sl->num_used-idx));
 +        sizeof(void*)*(sl->num_used-idx));

but if it's false, this other ugly thing happens:

    SMARTLIST_FOREACH(sl1, const char *, cp1, {
 -      const char *cp2 = smartlist_get(sl2, cp1_sl_idx);
 -      if (strcmp(cp1, cp2))
 -        return 0;
 -    });
 +                      const char *cp2 = smartlist_get(sl2, cp1_sl_idx);
 +                      if (strcmp(cp1, cp2))
 +                        return 0;
 +                    });

which is not the way we usually align such macros.
I left it on 'false' in my configuration.


  • It will not think twice before beautifying macro definitions, but it won't align the '\' newline escapes afterwards. I haven't managed to find a configuration option for this (indent_align_string doesn't seem to do it).
 #define SMARTLIST_FOREACH_END(var)              \
-    var = NULL;                                 \
+  var = NULL;                                 \
   } } while (0)

Missing features

I'll mention here some features I would have enjoyed, in case an uncrustify guru comes in and points me to the correct configuration option.

  • A way to specify whether we like the final "*/" of a comment in a new line or not:
    /** Advance the iterator <b>iter</b> a single step to the next entry, removing
     * the current entry, and return its new value.
     */
    

versus

/** Set *<b>keyp</b> and *<b>valp</b> to the current entry pointed to by
 * iter. */

  • A way to tell it whether we like aligned struct members or not, so that it can break them down if we don't:
    struct conn_t {
      config_t           *cfg;
      char               *peername;
      circuit_t          *circuit;
      struct bufferevent *buffer;
      enum listen_mode mode;
    };
    

Other comments

Most changes in my branch were caused by these two options:

# Whether to put a star on subsequent comment lines
cmt_star_cont                            = true    # false/true

and

# The number of blank lines after a block of variable definitions at the top of a function body.
# 0=no change (default)
nl_func_var_def_blk                      = 1        # number

I'm not sure if I like the former option (although tor seems to like it), and I think I like the latter. By disabling these two options, we should get a more conservative diff.

Also, if we end up using uncrustify, I'm not sure if we should use it on sha256.c (tor's checkSpace.pl doesn't check sha256.c either), or container.[ch] (maybe we should keep container.[ch] as similar to its tor version as possible).

Thoughts

I think I'll stop playing with uncrustify for today.

I'll do some more tests this week, and see whether I'm wrong with any of my complaints above (very likely).
I'll then make a new branch, with the new config and the new changes (and a better Makefile), and put it here on display. If we like it, we can add an uncrustify configuration file to obfsproxy. If we don't like it, we can use checkSpace.pl while waiting for uncrustify to become better (or till we hack the features we want in, ourselves...).

comment:5 Changed 8 years ago by arma

Component: Pluggable transportObfsproxy

comment:6 Changed 6 years ago by asn

Resolution: wontfix
Status: needs_reviewclosed

This ticket is about the old C-based obfsproxy. Closing.

Note: See TracTickets for help on using tickets.