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 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.
> + /* 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?
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: ")
^^^^^^^^^^^^
> + {
> + 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.
--
vda
