On 10/04/2010 10:11 AM, Nikola Pajkovsky wrote:
On 10/01/2010 04:00 PM, Jiri Moskovcak wrote:
> On 09/29/2010 01:17 PM, Nikola Pajkovsky wrote:
>> Signed-off-by: Nikola Pajkovsky<npajkovs(a)redhat.com>
>> ---
>> src/daemon/Daemon.cpp | 65
+++++++++++++++++++++++++-----------------------
>> 1 files changed, 34 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/daemon/Daemon.cpp b/src/daemon/Daemon.cpp
>> index 8ab9991..5a4db14 100644
>> --- a/src/daemon/Daemon.cpp
>> +++ b/src/daemon/Daemon.cpp
>> @@ -34,6 +34,7 @@
>> #include "CrashWatcher.h"
>> #include "Daemon.h"
>> #include "rpm.h"
>> +#include "parse_options.h"
>>
>> using namespace std;
>>
>> @@ -85,6 +86,10 @@ using namespace std;
>> * Both are sent as unicast to last client set by
set_client_name(name).
>> * If set_client_name(NULL) was done, they are not sent.
>> */
>> +static const char * const abrtd_usage[] = {
> - does it have to be static?
I don't want to accidentally collide with others module, also it says it will be
used
only here and there is no *extern*
exactly, if there is no *extern* then you don't need to use static as
it's already global, but it probably won't hurt...
>> + "abrtd [options]",
>> + NULL
>> +};
>>
>> CCommLayerServer* g_pCommLayer;
>>
>> @@ -855,9 +860,20 @@ static void sanitize_dump_dir_rights()
>> ensure_writable_dir(VAR_RUN"/abrt", 0755, "root");
>> }
>>
>> +static int daemonize_opt, syslog_opt, help_opt;
>> +static char *timeout_opt;
>> +
>> +static struct options abrtd_options[] = {
>> + OPT_BOOL( 'h' , "help",&help_opt, "Show this help
message"),
>> + OPT_BOOL( 'd' , 0,&daemonize_opt, "Do not
daemonize"),
>> + OPT_BOOL( 's' , 0,&syslog_opt, "Log to syslog even with
-d"),
>> + OPT_STRING( 't' , 0,&timeout_opt, "Exit after SEC seconds
of inactivity"),
>> + OPT_BOOL( 'v' , 0,&g_verbose, "Verbose"),
>> + OPT_END()
>> +};
>> +
>
> - same here, why static?
>
ditto
>> int main(int argc, char** argv)
>> {
>> - int opt;
>> int parent_pid = getpid();
>>
>> setlocale(LC_ALL, "");
>> @@ -874,38 +890,25 @@ int main(int argc, char** argv)
>> if (env_verbose)
>> g_verbose = atoi(env_verbose);
>>
>> - while ((opt = getopt(argc, argv, "dsvt:")) != -1)
>> + parse_opts(argc, argv, abrtd_options, abrtd_usage);
>> +
>> + if (help_opt)
>> + parse_usage_and_die(abrtd_usage, abrtd_options);
>> +
>> + if (daemonize_opt)
>> + daemonize = false;
>> +
>> + if (syslog_opt)
>> + start_syslog_logging();
>> +
>> + if (timeout_opt)
>> {
>> unsigned long ul;
>> -
>> - switch (opt)
>> - {
>> - case 'd':
>> - daemonize = false;
>> - break;
>> - case 's':
>> - start_syslog_logging();
>> - break;
>> - case 'v':
>> - g_verbose++;
>> - break;
>> - case 't':
>> - char *end;
>> - errno = 0;
>> - s_timeout = ul = strtoul(optarg,&end, 0);
>> - if (errno == 0&& *end == '\0'&&
ul<= INT_MAX)
>> - break;
>> - /* fall through to error */
>> - default:
>> - error_msg_and_die(
>> - "Usage: abrtd [-dsv] [-t SEC]\n"
>> - "\nOptions:"
>> - "\n\t-d\tDo not daemonize"
>> - "\n\t-s\tLog to syslog even with -d"
>> - "\n\t-t SEC\tExit after SEC seconds of inactivity"
>> - "\n\t-v\tVerbose"
>> - );
>> - }
>> + char *end;
>> + errno = 0;
>> + s_timeout = ul = strtoul(timeout_opt,&end, 0);
> - bad formatting -> missing ' ' before&end
>> + if (!(errno == 0&& *end == '\0'&& ul<=
INT_MAX))
> - bad formatting -> missing ' ' before&&
seems it's a problem on your receiver :)
> - I find this condition quite hard to read (because of the negation), so
> I propose to use:
> if(errno != 0 || *end != '\0' || ul>= INT_MAX)
why not...
>> + parse_usage_and_die(abrtd_usage, abrtd_options);
>> }
>>
>> putenv(xasprintf("ABRT_VERBOSE=%u", g_verbose));
>