[FFmpeg-devel] [PATCH 3/4] ffserver_config: improve error handling

[Reynaldo H. Verdejo Pinochet]

Hi

On 10/31/2014 11:00 PM, Lukasz Marek wrote:
> [..]
> @@ -356,9 +356,7 @@ static int ffserver_parse_config_global(FFServerConfig *config, const char *cmd,
>          if (!av_strcasecmp(cmd, "Port"))
>              WARNING("Port option is deprecated, use HTTPPort instead\n");
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        val = atoi(arg);
> -        if (val < 1 || val > 65536)
> -            ERROR("Invalid port: %s\n", arg);
> +        ffserver_set_int_param(&val, arg, 0, 1, 65535, config, line_num, "Invalid port: %s\n", arg);
>          if (val < 1024)
>              WARNING("Trying to use IETF assigned system port: %d\n", val);
>          config->http_addr.sin_port = htons(val);
> @@ -367,37 +365,38 @@ static int ffserver_parse_config_global(FFServerConfig *config, const char *cmd,
>              WARNING("BindAddress option is deprecated, use HTTPBindAddress instead\n");
>          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);
> +            ERROR("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, "RTSPPort")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        val = atoi(arg);
> -        if (val < 1 || val > 65536)
> -            ERROR("%s:%d: Invalid port: %s\n", arg);
> -        config->rtsp_addr.sin_port = htons(atoi(arg));
> +        ffserver_set_int_param(&val, arg, 0, 1, 65535, config, line_num, "Invalid port: %s\n", arg);
> +        config->rtsp_addr.sin_port = htons(val);
>      } else if (!av_strcasecmp(cmd, "RTSPBindAddress")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
>          if (resolve_host(&config->rtsp_addr.sin_addr, arg) != 0)
>              ERROR("Invalid host/IP address: %s\n", arg);
>      } else if (!av_strcasecmp(cmd, "MaxHTTPConnections")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        val = atoi(arg);
> -        if (val < 1 || val > 65536)
> -            ERROR("Invalid MaxHTTPConnections: %s\n", arg);
> +        ffserver_set_int_param(&val, arg, 0, 1, 65535, config, line_num, "Invalid MaxHTTPConnections: %s\n", arg);

I don't think we should be capping MaxHTTPConnections at 65535.
If there's a reason then FFServerConfig.nb_max_http_connections
type needs to be revised too?

>          config->nb_max_http_connections = val;
> +        if (config->nb_max_connections > config->nb_max_http_connections)
> +            ERROR("Inconsistent configuration: MaxClients(%d) > MaxHTTPConnections(%d)\n",
> +                  config->nb_max_connections, config->nb_max_http_connections);
>      } else if (!av_strcasecmp(cmd, "MaxClients")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        val = atoi(arg);
> -        if (val < 1 || val > config->nb_max_http_connections)
> -            ERROR("Invalid MaxClients: %s\n", arg);
> -        else
> -            config->nb_max_connections = val;
> +        ffserver_set_int_param(&val, arg, 0, 1, 65535, config, line_num, "Invalid MaxClients: %s\n", arg);
> +        config->nb_max_connections = val;
> +        if (config->nb_max_connections > config->nb_max_http_connections)
> +            ERROR("Inconsistent configuration: MaxClients(%d) > MaxHTTPConnections(%d)\n",
> +                  config->nb_max_connections, config->nb_max_http_connections);

Same as above

> [...]
> @@ -500,6 +499,9 @@ static int ffserver_parse_config_feed(FFServerConfig *config, const char *cmd, c
>          case 'G':
>              fsize *= 1024 * 1024 * 1024;
>              break;
> +        default:
> +            ERROR("Invalid file size: %s\n", arg);
> +            break;
>          }
>          feed->feed_max_size = (int64_t)fsize;
>          if (feed->feed_max_size < FFM_PACKET_SIZE*4)
> @@ -876,11 +878,15 @@ static int ffserver_parse_config_stream(FFServerConfig *config, const char *cmd,
>          stream->is_multicast = 1;
>          stream->loop = 1; /* default is looping */
>      } else if (!av_strcasecmp(cmd, "MulticastPort")) {
> +        int val;
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        stream->multicast_port = atoi(arg);
> +        ffserver_set_int_param(&val, arg, 0, 1, 65535, config, line_num, "Invalid MulticastPort: %s\n", arg);
> +        stream->multicast_port = val;
>      } else if (!av_strcasecmp(cmd, "MulticastTTL")) {
> +        int val;

Maybe declare val once at the beginning
of ffserver_parse_config_stream()

Rest looks OK.
Bests,

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