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

[Nicolas George]

Le nonidi 9 brumaire, an CCXXIII, Binathi Bingi a écrit :
> >From 0fb7dcf1f126bd137e2b2025c5cd6cff4af65801 Mon Sep 17 00:00:00 2001
> From: Binathi Bingi <binti179 at gmail.com>
> Date: Thu, 30 Oct 2014 01:14:08 +0530
> Subject: [PATCH] ffserver: enable back deamon mode
> 
> ---
>  ffserver.c        | 34 ++++++++++++++++++++++++++++++----
>  ffserver_config.c |  2 --
>  2 files changed, 30 insertions(+), 6 deletions(-)

Thanks for the submission.

I believe you forgot to update the docs (doc/ffserver.texi).

> 
> diff --git a/ffserver.c b/ffserver.c
> index ea2a2ae..96e312e 100644
> --- a/ffserver.c
> +++ b/ffserver.c
> @@ -236,7 +236,7 @@ static int rtp_new_av_stream(HTTPContext *c,
>                               HTTPContext *rtsp_c);
> 
>  static const char *my_program_name;

> -

I think the empty line could stay.

> +static int ffserver_daemon;
>  static int no_launch;
>  static int need_to_start_children;
> 
> @@ -3671,6 +3671,7 @@ static void handle_child_exit(int sig)
>  static void opt_debug(void)
>  {
>      config.debug = 1;

> +    ffserver_daemon = 1;

This is strange: debugging is precisely when you do not want ffserver to
fork into background. If daemon mode is enabled by default, debug mode
should probably disable it, but not the other way around.

>      snprintf(config.logfilename, sizeof(config.logfilename), "-");
>  }
> 
> @@ -3694,7 +3695,7 @@ int main(int argc, char **argv)
>  {
>      struct sigaction sigact = { { 0 } };
>      int ret = 0;

> -

The empty line can stay.

> +    ffserver_daemon = 1;

This is changing the default behaviour again, I am not sure this is a good
idea nowadays. We can probably leave that line out for the time being.

>      config.filename = av_strdup("/etc/ffserver.conf");
> 
>      parse_loglevel(argc, argv, options);
> @@ -3736,10 +3737,35 @@ int main(int argc, char **argv)
>      build_feed_streams();
> 
>      compute_bandwidth();

> -

Ditto for the empty line.

> +    if(ffserver_daemon){

Style nitpick: space between if/for/while/switch and parentheses and before
the brace.

> +    int ffserver_id = 0;
> +    pid_t sid = 0;
> +

> +    ffserver_id = fork(); // Create child process

Small nitpick: the comment seems useless.

> +
> +    if (ffserver_id < 0){

> +        printf("fork failed!\n"); //Indication of failure

This kind of message should go to stderr, never stdout. In FFmpeg code, use
av_log() for that.

Also, please include the error reason, using errno (probably through AVERROR
and av_err2str()).

And the comment seems useless too.

> +        exit(1);
> +    }
> +

> +    if(ffserver_id > 0){ //Parent process need to kill

I do not understand the comment. And ditto for spacing.

> +        exit(0);
> +    }
> +
> +    sid = setsid(); //set new session

> +    if(sid < 0){
> +        exit(1); //return failure

Ditto for spacing and comment.

> +    }
> +

> +    open ("/dev/null", O_RDWR);
> +
> +    if (strcmp(config.logfilename, "-") != 0) {
> +                close(1);
> +        }

This looks strange. If the purpose is to sanitize stdio after detaching,
there are some bits missing.

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

> -
> +

I have no idea why Git proposes to replace an empty line with an identical
empty line, but that does not matter.

>      if (http_server() < 0) {
>          http_log("Could not start server\n");
>          exit(1);
> diff --git a/ffserver_config.c b/ffserver_config.c
> index e44cdf7..e2c78d8 100644
> --- a/ffserver_config.c
> +++ b/ffserver_config.c
> @@ -358,8 +358,6 @@ static int ffserver_parse_config_global(FFServerConfig
> *config, const char *cmd,
>          ffserver_get_arg(arg, sizeof(arg), p);
>          if (resolve_host(&config->http_addr.sin_addr, arg) != 0)
>              ERROR("%s:%d: Invalid host/IP address: %s\n", arg);

> -    } else if (!av_strcasecmp(cmd, "NoDaemon")) {
> -        WARNING("NoDaemon option has no effect, you should remove it\n");

Superseded by:

> +    } else if (!av_strcasecmp(cmd, "NoDaemon")) {
> +            WARNING("NoDaemon option has no effect, you should remove
> it\n");

First, you may notice that your mail user agent has mangled the patch by
adding a line break before the end of the last line. That happens when
pasting a patch directly in the body of a mail with a bad mail user agent.
This is of no matter here because of the next point.

Second, this patch needs to be squashed with the previous one, so that they
are a single commit. If you had not yet committed, you could do that using
the --amend option to git commit. Since you have already committed, you must
use "git rebase -i master" (assuming you created a branch from the branch
called master).

Third, I do not think this exact version is correct. If you make the daemon
mode the default, then NoDaemon must not be ignored, it must have its
specified effect: turn daemon off; if you do not make the daemon mode the
default, then there must be an option to turn it on.

IMHO, the best is to have both Daemon and NoDaemon mode.

>      } else if (!av_strcasecmp(cmd, "RTSPPort")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
>          val = atoi(arg);

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141030/accb4a67/attachment.asc>