Re: [PATCH] add http.maxReceiveSpeed to limit git-receive-pack receiving speed

From: Xiaowen Xia <haoyurenzhuxia@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, peff@peff.net, worldhello.net@gmail.com,
	Xia XiaoWen <chenan.xxw@alibaba-inc.com>
Subject: Re: [PATCH] add http.maxReceiveSpeed to limit git-receive-pack receiving speed
Date: Sat, 21 Aug 2021 11:19:00 +0800	[thread overview]
Message-ID: <CAHLXgnYsQcfgddNHdH+geoczq8isgLaf-b3oLzjNS+m96N5ESg@mail.gmail.com> (raw)
In-Reply-To: <xmqq4kbl2sre.fsf@gitster.g>

Hi Junio:

Thank You so much for your reply!!

> > The default is unlimited, same if the value is 0 or negative.
> Let's error it out when the configuration gives a value that does
> not make sense instead. That way, we could in the future use some
> of these invalid values to signal special behaviour if we wanted to.
But this patch is similar to the `http.lowspeedlimit` and `http.lowspeedtime`.
And `http.lowspeedlimit` will not error out the negative values:

        if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
            curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
                 curl_low_speed_limit);
            curl_easy_setopt(result, CURLOPT_LOW_SPEED_TIME,
                 curl_low_speed_time);
        }

> We are likely be raising the floor versions of libcURL to 7.16.0 or
> even 7.19.4 soonish.
OK, I will remove the #if #endif block for libcurl 7.15.5.

> Let's error it out when the configuration gives a value that does
> not make sense instead.
The same as above.

> I wonder if ssize_t is overkill for our purpose, though.
You are right, I will change it to long type.

> This call, if you use anything but curl_off_t as the type for
> curl_max_receive_speed variable where it is declared, needs a cast,
You are right.

> Unlike curl_max_receive_speed that must be visible and
> understandable long haul in this file, the temporary string variable
> lives only during these handful of lines and shortened name is
> easier to see and understand what is going on.
OK.

Junio C Hamano <gitster@pobox.com> 于2021年8月20日周五 上午2:28写道：
>
> Xia XiaoWen <haoyurenzhuxia@gmail.com> writes:
>
> > Sometimes need to limit the receive speed of git `clone/fetch`
> > because of the limited network bandwidth, otherwise will prevent
> > other applications from using the network normally.
>
> No subject in these two half-sentences.
>
>     In order to avoid hogging all the available bandwidth, users may
>     want to limit the speed to receive traffic for "git clone" or
>     "git fetch".
>
> perhaps.
>
> > Add `http.maxReceiveSpeed` to limit `git-receive-pack` receiving
>
> "limit `git-receive-pack`'s" or "limit receiving speedk of ..."
>
> > speed, Can be overridden by `GIT_HTTP_MAX_RECEIVE_SPEED` eivironment
> > variable.
> >
> > The default is unlimited, same if the value is 0 or negative. The
>
> Let's error it out when the configuration gives a value that does
> not make sense instead.  That way, we could in the future use some
> of these invalid values to signal special behaviour if we wanted to.
>
> > default unit is Bytes/s, common unit suffixes of k, m, or g are supported.
>
> OK.
>
> > this configuration is valid for `clone`, `fetch`, `pull` commands of the
>
> s/this/This/
>
> > https protocol, and only supports libcurl 7.15.5 and above.
>
> We are likely be raising the floor versions of libcURL to 7.16.0 or
> even 7.19.4 soonish.  It probably would make it easier to allow it
> unconditionally (otherwise you'd probably need to implement error or
> warning messages when configuration is given but the libcURL version
> used is too old, etc.).
>
> > ---
> >  http.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/http.c b/http.c
> > index 8119247149..12030cf3bc 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -83,6 +83,9 @@ static const char *ssl_pinnedkey;
> >  static const char *ssl_cainfo;
> >  static long curl_low_speed_limit = -1;
> >  static long curl_low_speed_time = -1;
> > +#if LIBCURL_VERSION_NUM >= 0x070f05
> > +static ssize_t curl_max_receive_speed = -1;
>
> On cURL side, CURLOPT_MAX_RECV_SPEED_LARGE takes curl_off_t, which
> is typically off_t (which is signed).
>
> I wonder if ssize_t is overkill for our purpose, though.  Can't this
> be a plain vanilla "int" or perhaps "long", just like the variable
> defined above uses "long" for "speed"?  Or is 2gb/s too low to be
> practical and we must use a 64-bit type?
>
> > +#endif
> >  static int curl_ftp_no_epsv;
> >  static const char *curl_http_proxy;
> >  static const char *http_proxy_authmethod;
> > @@ -361,7 +364,12 @@ static int http_options(const char *var, const char *value, void *cb)
> >               curl_low_speed_time = (long)git_config_int(var, value);
> >               return 0;
> >       }
> > -
> > +#if LIBCURL_VERSION_NUM >= 0x070f05
> > +     if (!strcmp("http.maxreceivespeed", var)) {
> > +             curl_max_receive_speed = git_config_ssize_t(var, value);
>
> Check for nonsense values, so that we can later use them to mean
> something special.  It is good to remember is that you can always
> loosen the rules after you give your software to your users, but it
> is very hard to tighten the rules.  As you never need more than one
> way to specify "the default" (aka "unlimited"), reserving any
> non-positive value to mean the default is a design that is
> extensible poorly.
>
> I.e. insert something like
>
>                 if (curl_max_receive_speed < 0)
>                         die("negatigve number for %s: %s", var, value);
>
> here.
>
> > +             return 0;
> > +     }
> > +#endif
> >       if (!strcmp("http.noepsv", var)) {
> >               curl_ftp_no_epsv = git_config_bool(var, value);
> >               return 0;
> > @@ -974,6 +982,12 @@ static CURL *get_curl_handle(void)
> >                                curl_low_speed_time);
> >       }
> >
> > +#if LIBCURL_VERSION_NUM >= 0x070f05
> > +     if (curl_max_receive_speed > 0)
>
> The "result" handle was created anew in this function, so the
> distinction does not really matter in practrice, but since you are
> carefully initializing the variable to "-1" so that we can
> differentiate the case where it is unconfigured (hence we want to
> use the default) and it is set to zero (hence we want to use the
> default), it would be more consistent and future-proof if you also
> allowed 0 to be passed here, i.e.
>
>         if (curl_max_receive_speed >= 0)
>
> > +             curl_easy_setopt(result, CURLOPT_MAX_RECV_SPEED_LARGE,
> > +                              curl_max_receive_speed);
>
> This call, if you use anything but curl_off_t as the type for
> curl_max_receive_speed variable where it is declared, needs a cast,
> like in the example https://curl.se/libcurl/c/CURLOPT_MAX_RECV_SPEED_LARGE.html
>
> > +#endif
> > +
> >       curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
> >  #if LIBCURL_VERSION_NUM >= 0x071301
> >       curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
> > @@ -1105,6 +1119,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> >  {
> >       char *low_speed_limit;
> >       char *low_speed_time;
> > +#if LIBCURL_VERSION_NUM >= 0x070f05
> > +     char *max_receive_speed;
> > +#endif
> >       char *normalized_url;
> >       struct urlmatch_config config = { STRING_LIST_INIT_DUP };
> >
> > @@ -1196,6 +1213,11 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> >       low_speed_time = getenv("GIT_HTTP_LOW_SPEED_TIME");
> >       if (low_speed_time != NULL)
> >               curl_low_speed_time = strtol(low_speed_time, NULL, 10);
> > +#if LIBCURL_VERSION_NUM >= 0x070f05
> > +     max_receive_speed = getenv("GIT_HTTP_MAX_RECEIVE_SPEED");
> > +     if (max_receive_speed && !git_parse_ssize_t(max_receive_speed, &curl_max_receive_speed))
>
> Overlong line.
>
> Unlike curl_max_receive_speed that must be visible and
> understandable long haul in this file, the temporary string variable
> lives only during these handful of lines and shortened name is
> easier to see and understand what is going on.  Also, you can avoid
> repeated spelling out of the environment variable name by giving a
> constant for it near the top of this function, e.g.
>
>         static const char mrs_env[] = "GIT_HTTP_MAX_RECEIVE_SPEED";
>
> Then this part would become:
>
>         if (mrs) {
>                 if (!git_parse_ssize_t(mrs, &curl_max_receive_speed))
>                         die(_("invalid number for %s: %s", mrs_env, mrs);
>                 if (curl_max_receive_speed < 0)
>                         die(_("negative number for %s: %s", mrs_env, mrs);
>         }
>
> > +             warning("failed to parse GIT_HTTP_MAX_RECEIVE_SPEED: %s", max_receive_speed);
>
>
> Thanks.