Hi Michal,
please see the review below.
Michal Toman wrote:
And a second try, now with the patch really included :)
Dňa 17.08.2011 17:08, Michal Toman wrote / napísal(a):
> Retrace server's /settings action outputs following:
>
> running_tasks 0
> max_running_tasks 5
> max_packed_size 50
> max_unpacked_size 1024
> supported_formats application/x-tar application/x-xz-compressed-tar
> application/x-gzip
> supported_releases fedora-16-i386 fedora-15-x86_64 fedora-15-i386
> fedora-16-x86_64
>
> but the current retrace-client only handles numeric values. This
> patch
> enables format and release verification.
> Please review
>
> M.
diff --git a/src/plugins/abrt-retrace-client.c
b/src/plugins/abrt-retrace-client.c
index a19300e..80c77e7 100644
--- a/src/plugins/abrt-retrace-client.c
+++ b/src/plugins/abrt-retrace-client.c
+static int max_formats = 15;
+static int max_releases = 31;
Preprocessor can be used to provide fixed array sizes, so I would vote for them here:
#define MAX_FORMATS 15
#define MAX_RELEASES 31
You do not need to allocate the supported_* arrays then:
struct retrace_settings
{
int running_tasks;
int max_running_tasks;
long long max_packed_size;
long long max_unpacked_size;
char *supported_formats[MAX_FORMATS];
char *supported_releases[MAX_RELEASES];
};
+ struct retrace_settings *settings = xmalloc(sizeof(struct
retrace_settings));
/* defaults */
settings->running_tasks = 0;
settings->max_running_tasks = 0;
settings->max_packed_size = 0;
settings->max_unpacked_size = 0;
settings->supported_formats = NULL;
settings->supported_releases = NULL;
xzalloc sets all the retrace_setting fields to zero so they don't need to be
initialized manually
struct retrace_settings *settings = xzalloc(sizeof(struct retrace_settings));
+ /* skip multiple lines with same prefix */
+ if (settings->supported_formats)
+ {
+ row = c + 1;
+ continue;
+ }
+
+ settings->supported_formats = xmalloc((max_formats + 1) * sizeof(char
*));
+ settings->supported_formats[max_formats] = NULL;
This block can be removed when #defines and fixed array sizes are used.
+ for (i = 0; i < max_formats && (space =
strchr(value, ' ')) != NULL; ++i)
+ {
+ *space = '\0';
+ settings->supported_formats[i] = xstrdup(value);
+ value = space + 1;
+ }
+
+ /* last element */
+ settings->supported_formats[i] = xstrdup(value);
+ settings->supported_formats[++i] = NULL;
Here is a buffer overflow. Consider "i == max_formats" after the for loop.
supported_formats[++i] is then out of the array. With xzalloc and fixed size array all
members of that array are already NULL.
"(space = strchr(value, ' ')) != NULL" can be simplified to "(space
= strchr(value, ' '))"
+/* dirty, dirty, dirty */
I think that code is pretty good ;-)
+ char *result = NULL;
+ if (strcasestr(line, "fedora") != NULL)
+ {
+ int ver;
+ if (sscanf(version, "%d", &ver) != 1)
+ return NULL;
+
+ result = xasprintf("fedora-%d-%s", ver, arch);
+ }
+ else if (strcasestr(line, "red hat enterprise linux") != NULL)
+ {
+ int maj, min;
+ if (sscanf(version, "%d.%d", &maj, &min) != 2)
+ return NULL;
+
+ result = xasprintf("rhel-%d.%d-%s", maj, min, arch);
+ }
+
+ free(arch);
+ return result;
free(line) is missing before "return result".
if sscanf fails, both arch and line should be freed as well.
Otherwise it looks good.
I am attaching a patch with changes I made during the review. Feel free to use as you see
fit.
Thanks,
Karel