Dňa 13.04.2011 12:12, Karel Klíč wrote / napísal(a):

Michal Tomanmtoman@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