Denys Vlasenko <dvlasenk(a)redhat.com> writes:
On Fri, 2011-08-12 at 10:22 +0200, Nikola Pajkovsky wrote:
> + /* Is crash a dup? (In this case, is_crash_a_dup() should have
> + * aborted "post-create" event processing as soon as it saw uuid
> + * and determined that there is another crash with same uuid.
> + * In this case it sets state.crash_dump_dup_name)
> + */
> + if (!crash_dump_dup_name)
> + {
> + /* No. Was there error on one of processing steps in run_event? */
> + if (r != 0)
> + return r; /* yes */
> +
> + /* Was uuid created after all? (In this case, is_crash_a_dup()
> + * should have fetched it and created uuid)
> + */
> + if (!uuid)
> + {
> + /* no */
> + fprintf(stdout, "Dump directory '%s' has no UUID
element", dump_dir_name);
> + return 1;
> + }
> + }
> + else
> + {
> + fprintf(stdout, "DUP_OF_DIR: %s", crash_dump_dup_name);
(BTW, fprintf(stdout, ...) == printf(...), you can simply use printf).
I know about that...
I would output this line with terminating \n: "DUP_OF_DIR:
%s\n".
Working with unterminated last lines creates all kinds of problems
later.
Hmm,
fprintf(stdout, "Dump directory '%s' has no UUID element",
dump_dir_name);
above has the same problem...
Why wouldn't you just use log(...) instead? It appends "\n" itself,
and is shorter.
I didn't use log(...), because it writes on stderr and I don't care
about stderr. I'm just reading these messages from stdout and let stderr
prints out the rest debugging information. Otherwise I'd have to print
out eaten messages and then do parsing.
> + /* exit 0 means, this is a good, non-dup dir */
> + if (status != 0)
> {
> - dump_dir_name = state.crash_dump_dup_name;
> - }
> + /* exit != 0 and empty buffer means error processing in run_event */
> + if (buf_out->len == 0)
> + {
> + strbuf_free(buf_out);
> + return MW_ERROR;
> + }
This if() {...} is not necessary, the same will be done in
"else /* no DUP_OF_DIR */ ... " below.
Are do doing it just to make sure buf_out->len != 0 below?
yes
Then you can just delete the above block and rewrite
the block below to cope with buf_out->len == 0
(and while at it, make it strip multiple \n's if necessary):
> + if (buf_out->buf[buf_out->len - 1] == '\n')
> + buf_out->buf[buf_out->len - 1] = '\0';
while (buf_out->len != 0 && buf_out->buf[buf_out->len - 1] ==
'\n')
buf_out->buf[--buf_out->len] = '\0';
> + char *last_line = strrchr(buf_out->buf, '\n');
> + if (!last_line)
> + last_line = buf_out->buf;
>
> + if (strncmp("DUP_OF_DIR: ", last_line,
strlen("DUP_OF_DIR")) == 0)
Should be:
strncmp("DUP_OF_DIR: ", last_line, strlen("DUP_OF_DIR: ")
^^^^^^^^^^^^
Uff, fixed...
> + {
> + dump_dir_name = last_line + strlen("DUP_OF_DIR: ");
> + }
> + else /* no DUP_OF_DIR */
> + {
> + strbuf_free(buf_out);
> + return MW_ERROR;
> + }
> + }
Apart from these small things, looks ok to me.
new patch has been sent
--
Save the whales. Feed the hungry. Free the mallocs