[FFmpeg-devel] "OPW Qualification Task: Enable daemon mode for FFserver"

[Reynaldo H. Verdejo Pinochet]

Hi

On 11/03/2014 04:09 PM, Binathi Bingi wrote:
> Hello,
> 
> Inside the child process, I closed the file descriptor and then reopened
> and redirected them using dup2() and later closed the opened file. I am not
> sure if I understood and used the functionality of dup2() in the right
> sense.
> 

Looks about right but see bellow.

> [..]
>> @@ -3736,10 +3737,42 @@ int main(int argc, char **argv)
>>      build_feed_streams();
>>  
>>      compute_bandwidth();
>> -
>> +    

Remove trailing withe space

>> +    if (config.ffserver_daemon) {

What follows needs to be indented
	
>> +	pid_t ffserver_id = 0; 
>> +	pid_t sid = 0;
>> +	
>> +	ffserver_id = fork(); 
>> +
>> +	if (ffserver_id < 0) {
>> +            ret = AVERROR(errno);
>> +	    av_log(NULL, AV_LOG_ERROR, "Impossible to start in daemon mode: %s\n", av_err2str(ret)); 
>> +	    exit(1);

Fix wrong indentation

>> +	}
>> +
>> +	if (ffserver_id > 0) { 
>> +	    exit(0);
>> +	}

Drop the braces from single statement ifs blocks like this one.

>> +	
>> +	sid = setsid(); 
>> +	if (sid < 0) {
>> +	    exit(1); 
>> +	}

Same as above

>> +	
>> +	if (strcmp(config.logfilename, "-") != 0) {
>> +            close(0);

Drop the != 0. Anything but 0 will evaluate to true anyway. You do this
elsewhere on your own code. Be consistent.

>> +	
>> +	fd = open("/dev/null", O_RDWR);
>> +	dup2(fd,0);
>> +	dup2(fd,1);
>> +	dup2(fd,2);

You sure you wana dup2() stdin, stdout and stderr on the above
condition? Please double check.

>> +	close(fd);

Above block needs to be re-indented. Also, check for failures in
open and dup2() and react accordingly. I think Nicolas mentioned
this already.

>> +        
>> +        }
>> +    }
>>      /* signal init */
>>      signal(SIGPIPE, SIG_IGN);
>> -
>> +    

Again, drop the trailing white space you introduced here and on
multiple lines in this same patch.

It's getting there. Congrats ;)

Bests,

-- 
Reynaldo H. Verdejo Pinochet
Open Source Group
Samsung Research America / Silicon Valley