[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Sep 2014 01:31:16 +0530
From: Sayantan Datta <std2048@...il.com>
To: john-dev <john-dev@...ts.openwall.com>
Subject: Re: Restart work on mask mode
Hi magnum,
On Mon, Sep 22, 2014 at 2:09 AM, Solar Designer <solar@...nwall.com> wrote:
> Sayantan,
>
> On Mon, Sep 22, 2014 at 12:22:22AM +0400, Solar Designer wrote:
> > Just one detail I am actually able to communicate to you easily: please
> > don't use nested functions.
>
> Oh, here's some more:
>
> Please don't forget to add yourself as a copyright holder for this
> source file.
>
> This (commented out) line is weird:
>
> //#include <string.h> /* for isalpha(), isxdigit() */
>
> Those macros actually come from <ctype.h>, not <string.h>.
>
> These lines should have spaces added:
>
> #include<string.h>
> #include<stdlib.h>
>
> The indentation levels are excessive - ideally, you'd re-arrange the
> code such that you would not need to nest blocks this deeply.
>
> Closely related to the above: many lines are overly long. In the rest
> of JtR, I tried to keep lines in under 80 chars, and that's with 8 char
> tabs. Sure, this is often difficult to do, but it provides a reminder
> to restructure the code (usually to split out more portions of it into
> functions) before it becomes too complex.
>
> Some curly braces are mis-indented, most commonly before "else", e.g.:
>
> end_c[k] = strtol(str, NULL, 16);
> }
> else {
> dash_loc_beg[k] = j - 1;
>
> would be corrected to:
>
> end_c[k] = strtol(str, NULL, 16);
> } else {
> dash_loc_beg[k] = j - 1;
>
> You pass potentially negative numbers into ctype macros:
>
> static void init_cpu_mask(char *mask, [...]
> [...]
> isxdigit((int)mask[j - 2]) [...]
>
> This causes sign extension. You need to cast to (int)(unsigned char)
> (yes, two casts in a row), or avoid ctype macros there (but don't make
> the same error with your own array lookups!)
>
> In many places (but not in all), you have no whitespace between "if" or
> "for" and the following opening brace. Elsewhere in JtR (and in the
> rest of instances in this same source file), we use "if (" and "for (",
> etc. with the whitespace.
>
> The commented out portions of code need to be fully dropped, or at least
> turned into #if 0 ... #endif blocks. They are not comments.
>
> Alexander
>
Please test the new mask mode cracker and apply it to bleeding-jumbo, if
you are comfortable.
Regards,
Sayantan
Content of type "text/html" skipped
Download attachment "mask_mode.patch.tar.gz" of type "application/x-gzip" (4961 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
