gh-Cyan4973/xxHash
Watch
1
0
Fork
You've already forked xxHash
 0
Code Issues 7 Pull requests 3 Releases 26 Wiki Activity

Add command line switch "-c"/"--check" #53

Merged
t-mat merged 14 commits from wip/check-files into dev 2016-02-19 09:23:40 -06:00
Conversation 15 Commits 14 Files changed 2 +670 -28
t-mat commented 2016-01-20 09:22:04 -06:00 (Migrated from github.com)

This PR is related to @beckermr's PR #21. Since it's simple and has many possibility to extend such as multi processing by xargs, I don't say this PR replaces PR #21.

New command line switches

This PR adds the following command line switches

  • -c/--check : read XXH32/64 sums from FILEs and check them.
  • --quiet : Don't print OK.
  • --status : Don't output anything to stdout/stderr.
  • --strict : Treat any invalid line as error.
  • --warn : Show warning message for invalid line.
  • If all checksum procedures are succeeded, exit code is 0.
    • Otherwise exit code is 1.

Example

./xxhsum * > my.xxhash
cat my.xxhash
./xxhsum -c my.xxhash

./xxhsum -H0 * > my.xxhash32
cat my.xxhash32
./xxhsum -c my.xxhash32

Basic goal

  • xxhsum -c mimics md5sum -c.
    • If we have no idea, follow md5sum.
  • xxhsum -c should accept, all valid, non-empty output which generated by xxhsum <filenames> or xxhsum -H0 <filenames>.
  • xxhsum -c detects xxHash bit width by first valid xxhsum line. And it doesn't allow mixed bit width in same .xxh file.
  • xxhsum -c does't support little endian mode.

Todo

  • Add test for Travis.
  • Add help message.
  • Consider varialbe length line allocation instead of fixed length (MAX_LINE_LENGTH)

Since I don't review/test this PR much, any suggestion is welcome even cosmetic one.

This PR is related to @beckermr's PR #21. Since it's simple and has many possibility to extend such as multi processing by `xargs`, I don't say this PR replaces PR #21. ## New command line switches This PR adds the following command line switches - `-c`/`--check` : read XXH32/64 sums from FILEs and check them. - `--quiet` : Don't print OK. - `--status` : Don't output anything to stdout/stderr. - `--strict` : Treat any invalid line as error. - `--warn` : Show warning message for invalid line. - If all checksum procedures are succeeded, exit code is 0. - Otherwise exit code is 1. ## Example ``` ./xxhsum * > my.xxhash cat my.xxhash ./xxhsum -c my.xxhash ./xxhsum -H0 * > my.xxhash32 cat my.xxhash32 ./xxhsum -c my.xxhash32 ``` ## Basic goal - `xxhsum -c` mimics `md5sum -c`. - If we have no idea, follow `md5sum`. - `xxhsum -c` should accept, all valid, non-empty output which generated by `xxhsum <filenames>` or `xxhsum -H0 <filenames>`. - `xxhsum -c` detects xxHash bit width by first valid xxhsum line. And it doesn't allow mixed bit width in same `.xxh` file. - `xxhsum -c` does't support little endian mode. ## Todo - Add test for Travis. - Add help message. - Consider varialbe length line allocation instead of fixed length (`MAX_LINE_LENGTH`) Since I don't review/test this PR much, any suggestion is welcome even cosmetic one.
Cyan4973 commented 2016-01-20 09:56:10 -06:00 (Migrated from github.com)

It's a very great PR @t-mat !

A few minor things that could be improved before merge :

  • Already identified : add Travis CI tests.
    You don't need to cover every possible error case for a first release,
    but at least having the nominal scenario covered is a good thing.
    Just add a test in Makefile, which mimics your already provided examples,
    add the test within test-all tag, and it will be good to go.
  • Already identified : variable length line allocation
    It can indeed be an issue if some files get very long path, which is not that uncommon.
    Plus, it would make you free of some MAX_PATH or PATH_MAX macros you are detecting.
  • Note that the current PR fails the g++ compilation test
  • Do we absolutely need <inttypes.h> and PRIuMAX ?
    What's the impact on Windows platform ? (currently untested)
It's a very great PR @t-mat ! A few minor things that could be improved before merge : - Already identified : add Travis CI tests. You don't need to cover every possible error case for a first release, but at least having the nominal scenario covered is a good thing. Just add a test in `Makefile`, which mimics your already provided examples, add the test within `test-all` tag, and it will be good to go. - Already identified : variable length line allocation It can indeed be an issue if some files get very long path, which is not that uncommon. Plus, it would make you free of some `MAX_PATH` or `PATH_MAX` macros you are detecting. - Note that the current PR fails the `g++` compilation test - Do we absolutely need `<inttypes.h>` and `PRIuMAX` ? What's the impact on Windows platform ? (currently untested)
t-mat commented 2016-01-20 23:02:44 -06:00 (Migrated from github.com)

at least having the nominal scenario covered is a good thing.

Sure. I'll add 32/64 bit normal scenario with xxhash repository files, and try to add invalid or mismatch hash line.

It can indeed be an issue if some files get very long path

Right. I'll implement it. Should I keep it less than 32KiBytes or so?

Note that the current PR fails the g++ compilation test

Oops. PRIuMAX! I should change it first.

Do we absolutely need <inttypes.h> and PRIuMAX ?

No. It's my mistake. Since all of them are C99 feature, if we use standard C99, size_t and %zu pair is better choice. If compiler doesn't have it, we can fall back to C89/90 unsigned long and %lu pair.

#if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \
    (defined(_MSC_VER) && _MSC_VER >= 1800)
/* C99, MSVC 2013 or later */
#define XXHSUM_PRINTF_SIZE_FMT "zu"
typedef size_t XxhsumPrintfSizeType;

#else

/* Here, we have possibility to support some legacy compiler which has
   both of unsigned long long and %llu such as MSVC 2012 or ealier.
   But I'm not sure we should support them or not. */
/*
#if defined(SOME_LEGACY_COMPILER_WHICH_HAS_LLU)
#define XXHSUM_PRINTF_SIZE_FMT "llu"
typedef U64 XxhsumPrintfSizeType;
#endif
*/

/* C89/90 */
#define XXHSUM_PRINTF_SIZE_FMT "lu"
typedef U32 XxhsumPrintfSizeType;

#endif

DISPLAY("size: %" XXHSUM_PRINTF_SIZE_FMT " bytes\n", (XxhsumPrintfSizeType) size);
> at least having the nominal scenario covered is a good thing. Sure. I'll add 32/64 bit normal scenario with xxhash repository files, and try to add invalid or mismatch hash line. > It can indeed be an issue if some files get very long path Right. I'll implement it. Should I keep it less than 32KiBytes or so? > Note that the current PR fails the g++ compilation test Oops. `PRIuMAX`! I should change it first. > Do we absolutely need <inttypes.h> and PRIuMAX ? No. It's my mistake. Since all of them are C99 feature, if we use standard C99, `size_t` and `%zu` pair is better choice. If compiler doesn't have it, we can fall back to C89/90 `unsigned long` and `%lu` pair. ``` #if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \ (defined(_MSC_VER) && _MSC_VER >= 1800) /* C99, MSVC 2013 or later */ #define XXHSUM_PRINTF_SIZE_FMT "zu" typedef size_t XxhsumPrintfSizeType; #else /* Here, we have possibility to support some legacy compiler which has both of unsigned long long and %llu such as MSVC 2012 or ealier. But I'm not sure we should support them or not. */ /* #if defined(SOME_LEGACY_COMPILER_WHICH_HAS_LLU) #define XXHSUM_PRINTF_SIZE_FMT "llu" typedef U64 XxhsumPrintfSizeType; #endif */ /* C89/90 */ #define XXHSUM_PRINTF_SIZE_FMT "lu" typedef U32 XxhsumPrintfSizeType; #endif DISPLAY("size: %" XXHSUM_PRINTF_SIZE_FMT " bytes\n", (XxhsumPrintfSizeType) size); ```
Cyan4973 commented 2016-01-21 03:17:01 -06:00 (Migrated from github.com)

Should I keep it less than 32KiBytes or so?

For a single filename, yes that sounds reasonable.

we have possibility to support some legacy compiler which has both of unsigned long long and %llu such as MSVC 2012 or ealier. But I'm not sure we should support them or not

Yes we should.
What is unclear for me though is what we gain from using %zu.

> Should I keep it less than 32KiBytes or so? For a single filename, yes that sounds reasonable. > we have possibility to support some legacy compiler which has both of unsigned long long and %llu such as MSVC 2012 or ealier. But I'm not sure we should support them or not Yes we should. What is unclear for me though is what we gain from using `%zu`.
t-mat commented 2016-01-21 05:47:45 -06:00 (Migrated from github.com)

What is unclear for me though is what we gain from using %zu.

According to C99 (N1256): 7.19.6.1/6, it says:

z Specifies that a following d, i, o, u, x, or X conversion specifier applies
to a size_t or the corresponding signed integer type argument;
or that a following n conversion specifier applies to a pointer to
a signed integer type corresponding to size_t argument.

Since we can't say size_t is any specific primitive type such as usigned int, unsigned long, nor unsigned long long, using %zu for size_t provides better consistency than %iu, %du, %lu, %llu.
If we use %zu for C99/11 (and newer MSVC), it's simply safe and optimal. Also it gracefully reduces tedious and tricky work such as (reasonably but strange) MSVC versions and target platform(win32/64) detection, etc. After that, we can forget about modern platforms, and only focus on legacy ones.

For legacy platform, I think we can suppose they support %llu in highly possibility. Because xxHash already requires 64 bit or greater unsigned long long.
It means their standard library may support %llu specifier to provide better support for unsigned long long. For example MSVC 2005 or later support unsigned long long and %llu, but MSVC 2003 or earlier don't support unsigned long long and %llu and we can't compile xxhash for latter legacy compilers without patches.

So in the following new definition, I suppose basically platform which supports unsigned long long also supports %llu even they don't provide fully compliant C99 spec.
If we find legacy platform which only provide unsigned long long and don't support %llu specifier, we can add detection macro and definition easily.

#if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \
    (defined(_MSC_VER) && _MSC_VER >= 1800)

/* C99, MSVC 2013 or later */
#define XXHSUM_PRINTF_SIZE_FMT "zu"
typedef size_t XxhsumPrintfSizeType;

#elif defined(COMPILIERS_WHICH_HAS_UNSIGNEDLONGLONG_BUT_DONT_SUPPORT_LLU_SPECIFIER)

/* Fully C89/90 compliant specifier and type. */
#define XXHSUM_PRINTF_SIZE_FMT "lu"
typedef U32 XxhsumPrintfSizeType;

#else

#define XXHSUM_PRINTF_SIZE_FMT "llu"
typedef U64 XxhsumPrintfSizeType;

#endif

This is not a perfect solution, but I hope this form helps future PRs which try to port xxHash for their legacy platforms.

> What is unclear for me though is what we gain from using %zu. According to [C99 (N1256): 7.19.6.1/6](http://port70.net/~nsz/c/c99/n1256.html#7.19.6.1p6), it says: > z Specifies that a following d, i, o, u, x, or X conversion specifier applies > to a size_t or the corresponding signed integer type argument; > or that a following n conversion specifier applies to a pointer to > a signed integer type corresponding to size_t argument. Since we can't say `size_t` is any specific primitive type such as `usigned int`, `unsigned long`, nor `unsigned long long`, using `%zu` for `size_t` provides better consistency than `%iu`, `%du`, `%lu`, `%llu`. If we use `%zu` for C99/11 (and newer MSVC), it's simply safe and optimal. Also it gracefully reduces tedious and tricky work such as (reasonably but strange) MSVC versions and target platform(win32/64) detection, etc. After that, we can forget about modern platforms, and only focus on legacy ones. For legacy platform, I think we can suppose they support `%llu` in highly possibility. Because xxHash already requires 64 bit or greater `unsigned long long`. It means their standard library may support `%llu` specifier to provide better support for `unsigned long long`. For example [MSVC 2005 or later](https://msdn.microsoft.com/en-us/library/56e442dc%28v=vs.80%29.aspx) support `unsigned long long` and `%llu`, but [MSVC 2003 or earlier](https://msdn.microsoft.com/en-us/library/s3f49ktz%28v=vs.71%29.aspx) don't support `unsigned long long` and `%llu` and we can't compile xxhash for latter legacy compilers without patches. So in the following new definition, I suppose basically platform which supports `unsigned long long` also supports `%llu` even they don't provide fully compliant C99 spec. If we find legacy platform which only provide `unsigned long long` and don't support `%llu` specifier, we can add detection macro and definition easily. ``` #if (defined (__STDC_VERSION__) && __STDC_VERSION__ >= 199901L) || \ (defined(_MSC_VER) && _MSC_VER >= 1800) /* C99, MSVC 2013 or later */ #define XXHSUM_PRINTF_SIZE_FMT "zu" typedef size_t XxhsumPrintfSizeType; #elif defined(COMPILIERS_WHICH_HAS_UNSIGNEDLONGLONG_BUT_DONT_SUPPORT_LLU_SPECIFIER) /* Fully C89/90 compliant specifier and type. */ #define XXHSUM_PRINTF_SIZE_FMT "lu" typedef U32 XxhsumPrintfSizeType; #else #define XXHSUM_PRINTF_SIZE_FMT "llu" typedef U64 XxhsumPrintfSizeType; #endif ``` This is not a perfect solution, but I hope this form helps future PRs which try to port xxHash for their legacy platforms.
Cyan4973 commented 2016-01-21 05:56:32 -06:00 (Migrated from github.com)

I understand what %zu is about.

It's just that I'm not sure if it brings any value to use it.
I'm also not sure where you intend to use it, since the current PR doesn't mention %zu anywhere.

In which case does it make sense to display a value which must be size_t type, as opposed to some simpler U32 type ?

I understand what `%zu` is about. It's just that I'm not sure if it brings any value to use it. I'm also not sure where you intend to use it, since the current PR doesn't mention `%zu` anywhere. In which case does it make sense to display a value which must be `size_t` type, as opposed to some simpler `U32` type ?
t-mat commented 2016-01-21 06:19:51 -06:00 (Migrated from github.com)

It it is only matter of printf() size_t line number

size_t lineNumber = ...;

// current
DISPLAYRESULT("%s : %" PRIuMAX ": FAILED open or read %s\n"
              , inFileName, (uintmax_t) lineNumber, parsedLine.filename);

// %zu, size_t
DISPLAYRESULT("%s : %zu: FAILED open or read %s\n"
              , inFileName, lineNumber, parsedLine.filename);

// With new macro which supports { %zu, size_t }, { %llu, unsigned long long },
// { %lu, unsigned long } and so on.
DISPLAYRESULT("%s : %" XXHSUM_PRINTF_SIZE_FMT ": FAILED open or read %s\n"
              , inFileName, (XxhsumPrintfSizeType) lineNumber, parsedLine.filename);

Since md5sum has error checking code for size_t overflow (here is an actual md5sum code), I followed it in brain-dead mode.
But frankly I'm not sure there is a needs for huge line number or not (may be pipe mode?). So I think we can also simply do it with U32 lineNumber;.

It it is only matter of printf() [`size_t` line number](https://github.com/t-mat/xxHash/commit/f47e8e1ff84855b6fea8916f1f5f27d6c46c80a2#diff-5c1239042f1fd481e413eb578eae056bR887) ``` size_t lineNumber = ...; // current DISPLAYRESULT("%s : %" PRIuMAX ": FAILED open or read %s\n" , inFileName, (uintmax_t) lineNumber, parsedLine.filename); // %zu, size_t DISPLAYRESULT("%s : %zu: FAILED open or read %s\n" , inFileName, lineNumber, parsedLine.filename); // With new macro which supports { %zu, size_t }, { %llu, unsigned long long }, // { %lu, unsigned long } and so on. DISPLAYRESULT("%s : %" XXHSUM_PRINTF_SIZE_FMT ": FAILED open or read %s\n" , inFileName, (XxhsumPrintfSizeType) lineNumber, parsedLine.filename); ``` Since md5sum has [error checking code for size_t overflow](https://github.com/t-mat/xxHash/commit/f47e8e1ff84855b6fea8916f1f5f27d6c46c80a2#diff-5c1239042f1fd481e413eb578eae056bR897) (here is an [actual md5sum code](http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/md5sum.c?id=cfdac06196de4af636ddcfdbfb4c4fd22782d6e7#n566)), I followed it in brain-dead mode. But frankly I'm not sure there is a needs for huge line number or not (may be pipe mode?). So I think we can also simply do it with `U32 lineNumber;`.
Cyan4973 commented 2016-01-21 06:23:29 -06:00 (Migrated from github.com)

I agree. 4 billion lines ought to be enough for everyone ... ugh ...
Well, let's keep this overflow check just in case ...

I agree. 4 billion lines ought to be enough for everyone ... ugh ... Well, let's keep this overflow check just in case ...
t-mat commented 2016-01-21 06:54:07 -06:00 (Migrated from github.com)

Sure, it would be better for code and mental health :)
So since I realize uint32_t introduces same problem (It requires PRIu32. Also sizeof(uint32_t) != sizeof(unsigned int) in ILP64), I would like to use the following code:

// 32, 64bit or something.
unsigned long lineNumber = ...;

// %ul, unsigned long
DISPLAYRESULT("%s : %ul: FAILED open or read %s\n"
              , inFileName, lineNumber, parsedLine.filename);
Sure, it would be better for code and mental health :) So since I realize `uint32_t` introduces same problem (It requires `PRIu32`. Also sizeof(uint32_t) != sizeof(unsigned int) in ILP64), I would like to use the following code: ``` // 32, 64bit or something. unsigned long lineNumber = ...; // %ul, unsigned long DISPLAYRESULT("%s : %ul: FAILED open or read %s\n" , inFileName, lineNumber, parsedLine.filename); ```
Cyan4973 commented 2016-01-21 06:56:17 -06:00 (Migrated from github.com)

Looks good to me !

Looks good to me !
t-mat commented 2016-01-22 00:14:34 -06:00 (Migrated from github.com)

I've updated PR partially. Todos are:

  • Use unsigned long for line number.
  • Implement variable length line reader.
  • Add Travis test case.
  • Add usage message for usage() and usage_advanced().
I've updated PR partially. Todos are: - [X] Use `unsigned long` for line number. - [X] Implement variable length line reader. - [x] Add Travis test case. - [x] Add usage message for usage() and usage_advanced().
t-mat commented 2016-01-25 04:38:25 -06:00 (Migrated from github.com)

I've add stdin mode (xxhsum -c -) and Makefile target for testing.

  • "test-xxhsum-c" tests xxhsum -c functions.
    • Input from file, pipe, stdin.
    • Basic warning/error cases
      • XXH32/64 mixed format warning.
      • Hash mismatch error.
      • Failed open or read error.
  • "clean-xxhsum-c" removes temporary files for testing.

Here is an actual test log on Travis. Perhaps you feel it's bulky, if so I'll add --quiet option for basic testing commands. It prevents to output 'OK' lines.

There are potential problematic implementations:

  • .test.xxh32, .test.xxh64
    • I use dot prefix to simplify glob specifier ('*' ignores dot files).
    • Perhaps they make confusion for users.
  • "$?" shell variable and test(1)
    • Since this combination supports all exit code explicitly, I use it for error code checking.
    • Both variable and command are widely supported (Linux, FreeBSD, MacOS), but there is potential pitfall.
    • There are another possible solutions
      • "||", "&&" short circuit.
      • Shell's if..else command.
I've add stdin mode (`xxhsum -c -`) and Makefile target for testing. - "test-xxhsum-c" tests xxhsum -c functions. - Input from file, pipe, stdin. - Basic warning/error cases - XXH32/64 mixed format warning. - Hash mismatch error. - Failed open or read error. - "clean-xxhsum-c" removes temporary files for testing. Here is [an actual test log on Travis](https://travis-ci.org/Cyan4973/xxHash/builds/104592831#L345-L461). Perhaps you feel it's bulky, if so I'll add [--quiet](https://github.com/Cyan4973/xxHash/blob/ef40492536e952828726ca7a0fc8d229d5106734/xxhsum.c#L1069) option for basic testing commands. It prevents to output 'OK' lines. There are potential problematic implementations: - .test.xxh32, .test.xxh64 - I use dot prefix to simplify glob specifier ('*' ignores dot files). - Perhaps they make confusion for users. - "$?" shell variable and test(1) - Since this combination supports all exit code explicitly, I use it for error code checking. - Both variable and command are widely supported (Linux, FreeBSD, MacOS), but there is potential pitfall. - There are another possible solutions - "||", "&&" short circuit. - Shell's if..else command.
Cyan4973 commented 2016-01-25 08:20:42 -06:00 (Migrated from github.com)

Here is an actual test log on Travis. Perhaps you feel it's bulky

That's fine, it's for tests.

I use dot prefix to simplify glob specifier ('*' ignores dot files).

Looks fine to me.

"$?" shell variable and test(1)

Looks good to me too.

"clean-xxhsum-c" removes temporary files for testing.

It could be run automatically at the end of the test.

> Here is an actual test log on Travis. Perhaps you feel it's bulky That's fine, it's for tests. > I use dot prefix to simplify glob specifier ('*' ignores dot files). Looks fine to me. > "$?" shell variable and test(1) Looks good to me too. > "clean-xxhsum-c" removes temporary files for testing. It could be run automatically at the end of the test.
Cyan4973 commented 2016-02-19 08:55:50 -06:00 (Migrated from github.com)

Is it ready for merge @t-mat ?
Or do you still want to complete "Add usage message for usage() and usage_advanced()." ?

Is it ready for merge @t-mat ? Or do you still want to complete "Add usage message for usage() and usage_advanced()." ?
t-mat commented 2016-02-19 09:22:16 -06:00 (Migrated from github.com)

Is it ready for merge @t-mat ?

oops. I've waited Travis, and yes, it's ready to merge 👍

I've read some md5sum related documents, and it seems good enough for now.
Maybe we'll need manpage for xxhsum but it should be another PR.

> Is it ready for merge @t-mat ? oops. I've waited Travis, and yes, it's ready to merge :+1: I've read some `md5sum` related documents, and it seems good enough for now. Maybe we'll need manpage for `xxhsum` but it should be another PR.
Cyan4973 commented 2016-02-19 09:23:36 -06:00 (Migrated from github.com)

👍

:+1:
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
announcement

   
bug

   
build

   
CI
Continuous Integration tests

  
dependencies
Pull requests that update a dependency file

  
documentation

   
duplicate

   
enhancement

   
experience feedback

   
feature request

   
github_actions
Pull requests that update GitHub Actions code

  
help wanted

   
install

   
invalid

   
Long-term

   
optimization

   
portability

   
question

   
target-specific
issue triggered in a specific environment (harware, compiler, OS)

  
test issue

   
wontfix
No labels
announcement
bug
build
CI
dependencies
documentation
duplicate
enhancement
experience feedback
feature request
github_actions
help wanted
install
invalid
Long-term
optimization
portability
question
target-specific
test issue
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Cyan4973/xxHash!53
No description provided.