When working on the code, I found a number of issues and odd behaviors. I tried to minimize the number of changes in my big push that added the libpar2.a library. Do we want to make any of these fixes?
-
Wildcard matching does not work.
Par2 does its own wildcard matching. I imagine that is so you can run it recursively with the "-R" option and find all your mp3 files. I found that patterns like "input1*.txt" and "ipt*.txt" did not match the file "input1.txt".
-
Wildcards that match 0 files do not produce an error (or even warning!).
If you use the pattern "*.mp3" on the commandline and no files match it, there is no error or even a warning to say that intended files were not found.
-
Our estimates of disk usage are not good for small recovery sets
If you ask for par2 files that fill 1024 bytes of disk space, the program uses 4144 bytes! For larger files, the disk usage may be an under-estimate.
-
Options must appear before filenames
It's convention that most options are before filenames, but par2 requires it. If you do "par2 create foo.par2 -v input1.txt", it will assume "-v" is a filename, not an option! We have a specific option called "--" that lets us add files that have a prefix of "-". We shouldn't disable options after the first filename.
-
Many options aren't checked well
When I do "-Bdirectory" or -btext", I don't get an error and the "directory" part is ignored. The correct format of the options has a space: "-B directory" or a number "-b200".
-
We may mishandle blocksizes for small recovery sets
In commandline.cpp, when (blockcount > totalsize) then blocksize=4. This should be when (blockcount*4 >= totalsize), right?
-
Files of length 0 are ignored.
They are fine by the spec. They can exist in either the recovery or non-recovery section. I don't know why they are skipped.
-
You can use the -a option when doing "repair" or "verify"
I'm not sure why you need this then.
-
default basepath is strange
If you don't specify a basepath, it's the path of the first file. Why? Shouldn't it be the path that is common to all input files?
-
location of par2 output file
Most programs that create a file put it in the current directory. However, if the first input file is in a different directory, the par2 file is written there. Why?
-
reedsolomon.cpp code allows parpresent > datamissing
This should probably be checked to make sure that parpresent=datamissing. That is, that the number of parity blocks used exactly matches the number of missing input blocks. I don't think the code correctly handles the care when parpresent is strictly greater than datamissing.
-
reedsolomon.cpp / galois.cpp doesn't handle negatives
This is a minor math detail, but reedsolomon.cpp should work with any type that obeys the property of a Field. Ours does not. It assumes we're using a Galois power-2 Field, which has the property that -a = a. So, if we change away from that for any reason, it won't work.
Suggestions:
-
UpdateVerificationResults is called more times than necessary in par2repairer.cpp
-
Galois.h's function Alog should take an int and return a Galois.
-
reedsolomon.h has a variable outputcount, which probably can be replaced with outputrows.size()
-
parfilename is a /complete/path/filename.par2 when we do "create", but "filename.par2" when running repair. Why the difference?
-
To create a parfile of 10MB, I use the option "-rm10". Wouldn't "-r10m" make more sense?
When working on the code, I found a number of issues and odd behaviors. I tried to minimize the number of changes in my big push that added the libpar2.a library. Do we want to make any of these fixes?
Wildcard matching does not work.
Par2 does its own wildcard matching. I imagine that is so you can run it recursively with the "-R" option and find all your mp3 files. I found that patterns like "input1*.txt" and "ipt*.txt" did not match the file "input1.txt".
Wildcards that match 0 files do not produce an error (or even warning!).
If you use the pattern "*.mp3" on the commandline and no files match it, there is no error or even a warning to say that intended files were not found.
Our estimates of disk usage are not good for small recovery sets
If you ask for par2 files that fill 1024 bytes of disk space, the program uses 4144 bytes! For larger files, the disk usage may be an under-estimate.
Options must appear before filenames
It's convention that most options are before filenames, but par2 requires it. If you do "par2 create foo.par2 -v input1.txt", it will assume "-v" is a filename, not an option! We have a specific option called "--" that lets us add files that have a prefix of "-". We shouldn't disable options after the first filename.
Many options aren't checked well
When I do "-Bdirectory" or -btext", I don't get an error and the "directory" part is ignored. The correct format of the options has a space: "-B directory" or a number "-b200".
We may mishandle blocksizes for small recovery sets
In commandline.cpp, when (blockcount > totalsize) then blocksize=4. This should be when (blockcount*4 >= totalsize), right?
Files of length 0 are ignored.
They are fine by the spec. They can exist in either the recovery or non-recovery section. I don't know why they are skipped.
You can use the -a option when doing "repair" or "verify"
I'm not sure why you need this then.
default basepath is strange
If you don't specify a basepath, it's the path of the first file. Why? Shouldn't it be the path that is common to all input files?
location of par2 output file
Most programs that create a file put it in the current directory. However, if the first input file is in a different directory, the par2 file is written there. Why?
reedsolomon.cpp code allows parpresent > datamissing
This should probably be checked to make sure that parpresent=datamissing. That is, that the number of parity blocks used exactly matches the number of missing input blocks. I don't think the code correctly handles the care when parpresent is strictly greater than datamissing.
reedsolomon.cpp / galois.cpp doesn't handle negatives
This is a minor math detail, but reedsolomon.cpp should work with any type that obeys the property of a Field. Ours does not. It assumes we're using a Galois power-2 Field, which has the property that -a = a. So, if we change away from that for any reason, it won't work.
Suggestions:
UpdateVerificationResults is called more times than necessary in par2repairer.cpp
Galois.h's function Alog should take an int and return a Galois.
reedsolomon.h has a variable outputcount, which probably can be replaced with outputrows.size()
parfilename is a /complete/path/filename.par2 when we do "create", but "filename.par2" when running repair. Why the difference?
To create a parfile of 10MB, I use the option "-rm10". Wouldn't "-r10m" make more sense?