Dňa 13.04.2011 12:12, Karel Klíč wrote / napísal(a):
Michal Toman<mtoman(a)redhat.com> writes:
> The first part of the patch - retrace client should be able to handle
> running_tasks, max_running_tasks, max_packed_size and
> max_unpacked_size options.
>
> Please review.
Looks good, thanks. I have found two minor issues:
1) A message says "the retrace server accepts crashes smaller than x
bytes" on two places, but the contition accepts values "smaller _or
equal_ to x bytes". It seems that there should be ">=" instead of
">"
in the if condition.
+ if (unpacked_size> settings.max_unpacked_size)
+ {
+ error_msg_and_die("The size of your crash is %lld bytes, "
+ "but the retrace server only accepts "
+ "crashes smaller than %lld bytes.",
+ unpacked_size, settings.max_unpacked_size);
+ }
+ if ((long long)file_stat.st_size> settings.max_packed_size)
+ {
+ error_msg_and_die("The size of your archive is %lld bytes, "
+ "but the retrace server only accepts "
+ "archives smaller than %lld bytes.",
+ (long long)file_stat.st_size,
+ settings.max_packed_size);
+ }
The condition is correct: <= means ok, > means error. The message should
be _or equal_.
2) The patch introduces the required_files variable. It would make sense
to use this variable not only to check the size of unpacked archive, but
also to actually create the archive, so we can modify only one place
when adding/removing files.
So the following code should be using required_files instead of
hardcoding the file names:
tar_args[argindex++] = FILENAME_COREDUMP;
args_add_if_exists(tar_args, dd, FILENAME_EXECUTABLE,&argindex);
args_add_if_exists(tar_args, dd, FILENAME_PACKAGE,&argindex);
args_add_if_exists(tar_args, dd, FILENAME_OS_RELEASE,&argindex);
Karel
Fixed, please re-review :).
Michal