Re: powerd

Nate Lawson <nate_at_root.org> writes:
> Dag-Erling Smørgrav wrote:
>> +static enum {
>> +	ac_none,
>> +	ac_acpi_sysctl,
>> +	ac_acpi_devd,
>> +#ifdef USE_APM
>> +	ac_apm,
>> +#endif
>> +} acline_mode;
>
> Prefer enums be all CAPS, seems like ac_apm can be left in the enum
> without an #ifdef because it's only checked by code in an #ifdef below.

Having the #ifdef here allows the compiler to warn us if we forget it
somewhere else.

>> +#ifdef __i386__
>> +	} else if ((apm_fd = open(APMDEV, O_RDONLY)) >= 0) {
>> +		if (vflag)
>> +			warnx("using APM for AC line status");
>> +		acline_mode = ac_apm;
>> +#endif
>
> Don't you want to use your new USE_APM define here?

Yes, that was an oversight.

>>  -static int
>> -acline_read()
>> +static void
>> +acline_read(void)
>
> Is this correct?  I thought only the prototype (above) should have
> void as an arg.

Yes, and no.

>>   static void
>>  devd_close(void)
>>  {
>>  -	if (devd_pipe < 0)
>> -		return;
>> -
>> -	pthread_kill(devd_thread, SIGTERM);
>>  	close(devd_pipe);
>> -}
>
> Seems like this function can go away now and we can just conditionally
> close devd_pipe (if != -1) at the end of main().

actually, devd_close() should include the line 'devd_pipe = -1;' and
all instances of close(devd_pipe) should be replaced with
devd_close().

>> -	/*
>> -	 * Exit cleanly on signals; devd may send a SIGPIPE if it dies.  We
>> -	 * do this before acline_init() since it may create a thread and we
>> -	 * want it to inherit our signal mask.
>> -	 */
>> -	signal(SIGINT, handle_sigs);
>> -	signal(SIGTERM, handle_sigs);
>> -	signal(SIGPIPE, SIG_IGN);
>> -
>
> Don't we still need SIGPIPE handling if reading from devd?

des_at_xps ~% man signal | grep SIGPIPE
     13    SIGPIPE      terminate process    write on a pipe with no reader

DES
-- 
Dag-Erling Smørgrav - des_at_des.no