[v2,1/1] remote-curl.c: xcurl_off_t is not portable (on 32 bit platfoms)

Commit Message

Torsten Bögershausen Nov. 9, 2018, 5:41 p.m. UTC

From: Torsten Bögershausen <[e-mail]>

When  setting
DEVELOPER = 1
DEVOPTS = extra-all

"gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516" errors out with
"comparison is always false due to limited range of data type"
"[-Werror=type-limits]"

It turns out that the function xcurl_off_t() has 2 flavours:
- It gives a warning 32 bit systems, like Linux
- It takes the signed ssize_t as a paramter, but the only caller is using
  a size_t (which is typically unsigned these days)

The original motivation of this function is to make sure that sizes > 2GiB
are handled correctly. The curl documentation says:
"For any given platform/compiler curl_off_t must be typedef'ed to a 64-bit
 wide signed integral data type"
On a 32 bit system "size_t" can be promoted into a 64 bit signed value
without loss of data, and therefore we may see the
"comparison is always false" warning.
On a 64 bit system it may happen, at least in theory, that size_t is > 2^63,
and then the promotion from an unsigned "size_t" into a signed "curl_off_t"
may be a problem.

One solution to suppress a possible compiler warning could be to remove
the function xcurl_off_t().

However, to be on the very safe side, we keep it and improve it:
- The len parameter is changed from ssize_t to size_t
- A temporally variable "size" is used, promoted int uintmax_t and the comopared
  with "maximum_signed_value_of_type(curl_off_t)".
  Thanks to Junio C Hamano for this hint.

Signed-off-by: Torsten Bögershausen <[e-mail]>
---

This is a re-semd, the orignal patch was part of a 2
patch-series.
This patch needed some rework, and here should be
the polished version.

remote-curl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Nov. 12, 2018, 3:50 a.m. UTC | #1

[e-mail] writes:

>
> This is a re-semd, the orignal patch was part of a 2
> patch-series.
> This patch needed some rework, and here should be
> the polished version.

Will queue.  Next time, please refrain from saying "re-send", if you
changed anything in the patch (or the log message), as the phrase
implies you are sending the same thing as before (just in case the
earlier one was not seen, for example).  Marking it as vN+1 like you
did for this patch and having a reference to the original would make
it clear, though ;-)

Thanks.

> remote-curl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 762a55a75f..1220dffcdc 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -617,10 +617,11 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  	return err;
>  }
>  
> -static curl_off_t xcurl_off_t(ssize_t len) {
> -	if (len > maximum_signed_value_of_type(curl_off_t))
> +static curl_off_t xcurl_off_t(size_t len) {
> +	uintmax_t size = len;
> +	if (size > maximum_signed_value_of_type(curl_off_t))
>  		die("cannot handle pushes this big");
> -	return (curl_off_t) len;
> +	return (curl_off_t)size;
>  }
>  
>  static int post_rpc(struct rpc_state *rpc)

Torsten Bögershausen Nov. 12, 2018, 5:20 a.m. UTC | #2

On Mon, Nov 12, 2018 at 12:50:30PM +0900, Junio C Hamano wrote:
> [e-mail] writes:
> 
> >
> > This is a re-semd, the orignal patch was part of a 2
> > patch-series.
> > This patch needed some rework, and here should be
> > the polished version.
> 
> Will queue.

Thanks, is there a chance to kill a typo here ?
s/comopared/compared/
- A temporally variable "size" is used, promoted int uintmax_t and the comopared


> Next time, please refrain from saying "re-send", if you
> changed anything in the patch (or the log message), as the phrase
> implies you are sending the same thing as before (just in case the
> earlier one was not seen, for example).  Marking it as vN+1 like you
> did for this patch and having a reference to the original would make
> it clear, though ;-)

Sorry for the confusion.
The next time I will not send unrelated patches as a series,
so that we have a better "Message-ID:" and "In-Reply-To:"
flow (which should make live 3% easier).

https://public-inbox.org/git/[e-mail]/
https://public-inbox.org/git/[e-mail]/

Junio C Hamano Nov. 12, 2018, 8:03 a.m. UTC | #3

Torsten Bögershausen <[e-mail]> writes:

> Thanks, is there a chance to kill a typo here ?
> s/comopared/compared/
> - A temporally variable "size" is used, promoted int uintmax_t and the comopared

Done.  Thanks.

Patch

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..1220dffcdc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -617,10 +617,11 @@  static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 	return err;
 }
 
-static curl_off_t xcurl_off_t(ssize_t len) {
-	if (len > maximum_signed_value_of_type(curl_off_t))
+static curl_off_t xcurl_off_t(size_t len) {
+	uintmax_t size = len;
+	if (size > maximum_signed_value_of_type(curl_off_t))
 		die("cannot handle pushes this big");
-	return (curl_off_t) len;
+	return (curl_off_t)size;
 }
 
 static int post_rpc(struct rpc_state *rpc)