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

[Nicolas George]

Le decadi 10 brumaire, an CCXXIII, Binathi Bingi a écrit :
> Hello
> 
> I tried to include the changes specified by Nicholas.
> We can switch between both Daemon and NoDaemon mode, using the option in
> ffserver.conf file.
> 
> >From 018f8c1e1acf062a9e6a3ec94f671d574ec4b712 Mon Sep 17 00:00:00 2001
> From: Binathi <binti179 at gmail.com>
> Date: Fri, 31 Oct 2014 23:27:20 +0530
> Subject: [PATCH] Enable Daemon mode for FFServer
> 
> Signed-off-by: Binathi <binti179 at gmail.com>
> ---
>  doc/ffserver.conf |  4 ++++
>  doc/ffserver.texi |  7 +++++--
>  ffserver.c        | 31 +++++++++++++++++++++++++++++--
>  ffserver_config.c |  7 +++++--
>  ffserver_config.h |  1 +
>  5 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/ffserver.conf b/doc/ffserver.conf
> index b756961..eac088b 100644
> --- a/doc/ffserver.conf
> +++ b/doc/ffserver.conf
> @@ -25,6 +25,10 @@ MaxBandwidth 1000
>  # '-' is the standard output.
>  CustomLog -
> 

> +# Suppress Daemon if you don't want to launch ffserver in daemon mode.
> +#NoDaemon
> +Daemon

Needs to be consistent with the default, see below.

> +
>  ##################################################################
>  # Definition of the live feeds. Each live feed contains one video
>  # and/or audio sequence coming from an ffmpeg encoder or another
> diff --git a/doc/ffserver.texi b/doc/ffserver.texi
> index 77273d2..15dc4b3 100644
> --- a/doc/ffserver.texi
> +++ b/doc/ffserver.texi
> @@ -406,8 +406,11 @@ ignored, and the log is written to standard output.
> 
>  @item NoDaemon

>  Set no-daemon mode. This option is currently ignored since now
> - at command{ffserver} will always work in no-daemon mode, and is
> -deprecated.
> + at command{ffserver} will always work in daemon mode.
> +
> +@ Daemon
> +Set daemon mode.
> + at command{ffserver} will always work in daemon mode. To enable no-daemon
> mode, suppress this and enable NoDaemon.

The wording is misleading: ffserver will NOT "always" work in daemon mode:
the options are precisely there to choose if it does. The usual wording for
that kind of option look usually like that:

Option_foo: make the application behave foo; this is the default.

Option_bar: make the application behave bar; the default is foo.

>  @end table
> 
>  @section Feed section
> diff --git a/ffserver.c b/ffserver.c
> index ea2a2ae..d6eb0b4 100644
> --- a/ffserver.c
> +++ b/ffserver.c
> @@ -3671,6 +3671,7 @@ static void handle_child_exit(int sig)
>  static void opt_debug(void)
>  {
>      config.debug = 1;
> +    config.ffserver_daemon = 0;
>      snprintf(config.logfilename, sizeof(config.logfilename), "-");
>  }
> 
> @@ -3736,10 +3737,36 @@ int main(int argc, char **argv)
>      build_feed_streams();
> 
>      compute_bandwidth();
> -
> +

> +    if (config.ffserver_daemon) {

Do not forget to indent.

> +    int ffserver_id = 0;
> +    pid_t sid = 0;
> +
> +    ffserver_id = fork();
> +
> +    if (ffserver_id < 0) {

> +        av_log(NULL, AV_LOG_WARNING, "Fork failed!Couldn't launch ffserver
> in daemon mode.\n");
> +        exit(1);

Since there is exit(1), then this is an ERROR, not a WARNING.

And please translate the reason for the fork fail:
av_err2str(AVERROR(errno)).

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

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

This looked wrong before and still looks wrong now. Opening a file (or
device) and ignoring the returned file descriptor is almost never correct.
Closing stdout and not re-opening it afterwards can hardly be right either.

> +    }
>      /* signal init */
>      signal(SIGPIPE, SIG_IGN);
> -
> +
>      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..f46d8f4 100644
> --- a/ffserver_config.c
> +++ b/ffserver_config.c
> @@ -358,8 +358,11 @@ 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");

> +    } else if (!av_strcasecmp(cmd, "Daemon") || !av_strcasecmp(cmd,
> "NoDaemon")) {
> +        if (!av_strcasecmp(cmd, "Daemon"))
> +            config->ffserver_daemon = 1;
> +        if (!av_strcasecmp(cmd, "NoDaemon"))
> +            config->ffserver_daemon = 0;

As Michael pointed out, no need to group them in a nested test, you can just
treat them as two separate options:

	    ...
	else if Daemon
	    ffserver_daemon = 1;
	else if NoDaemon
	    ffserver_daemon = 0;
	else ...

>      } else if (!av_strcasecmp(cmd, "RTSPPort")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
>          val = atoi(arg);
> diff --git a/ffserver_config.h b/ffserver_config.h
> index 36d61d0..e3957b1 100644
> --- a/ffserver_config.h
> +++ b/ffserver_config.h
> @@ -100,6 +100,7 @@ typedef struct FFServerConfig {
>      unsigned int nb_max_http_connections;
>      unsigned int nb_max_connections;
>      uint64_t max_bandwidth;

> +    int ffserver_daemon;

You define that field, but you never init it: that means the initial value
is 0. In other words, the default when no option is specified is NoDaemon.
This is very fine, since this is currently the default and it is better not
to change it. But the documentation (both the example snippet and the
reference manual) must state it accordingly.

>      int debug;
>      char logfilename[1024];
>      struct sockaddr_in http_addr;

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/20141101/1535ada9/attachment.asc>