threads / patch / 8005

patchconnect: display connection progress

Subject: [PATCH] connect: display connection progress

## tl;dr

21 messages between May 6, 2007 and May 10, 2007. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

Michael S. Tsirkin· May 6, 2007, 19:52 UTC · lore

Make git notify the user about host resolution/connection attempts. This is useful both as a progress indicator on slow links, and helps reassure the user there are no DNS/firewall problems.

Signed-off-by: Michael S. Tsirkin <mst@dev.mellanox.co.il>
---

I find the following useful. This currently only covers native git protocol. I expect it would be easy to extend this to other protocols, if there's interest. Opinions?

Show changes to connect.c +4 −0
diff --git a/connect.c b/connect.c
index da89c9c..f026713 100644
--- a/connect.c
+++ b/connect.c
@@ -425,9 +425,11 @@ static int git_tcp_connect_sock(char *host)
 	hints.ai_socktype = SOCK_STREAM;
 	hints.ai_protocol = IPPROTO_TCP;
 
+	fprintf(stderr, "Looking up %s ... ", host);
 	gai = getaddrinfo(host, port, &hints, &ai);
 	if (gai)
 		die("Unable to look up %s (port %s) (%s)", host, port, gai_strerror(gai));
+	fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
 
 	for (ai0 = ai; ai; ai = ai->ai_next) {
 		sockfd = socket(ai->ai_family,
@@ -450,6 +452,8 @@ static int git_tcp_connect_sock(char *host)
 	if (sockfd < 0)
 		die("unable to connect a socket (%s)", strerror(saved_errno));
 
+	fprintf(stderr, "done.\n");
+
 	return sockfd;
 }
 
-- 
MST
Junio C Hamano· May 6, 2007, 20:41 UTC · re: Michael S. Tsirkin · lore

Re: [PATCH] connect: display connection progress

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
Show 12 quoted lines
> Make git notify the user about host resolution/connection attempts.  This
> is useful both as a progress indicator on slow links, and helps reassure the
> user there are no DNS/firewall problems.
>
> Signed-off-by: Michael S. Tsirkin <mst@dev.mellanox.co.il>
>
> ---
>
> I find the following useful.
> This currently only covers native git protocol. I expect it would
> be easy to extend this to other protocols, if there's interest.
> Opinions?

I think giving this kind of feedback makes a lot of sense, from both the "assurance" point of view and also debuggability.

But please do this only under verbose, or squelch it if "quiet" is asked.

Michael S. Tsirkin· May 7, 2007, 04:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] connect: display connection progress

Show 23 quoted lines
> Quoting Junio C Hamano <junkio@cox.net>:
> Subject: Re: [PATCH] connect: display connection progress
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> 
> > Make git notify the user about host resolution/connection attempts.  This
> > is useful both as a progress indicator on slow links, and helps reassure the
> > user there are no DNS/firewall problems.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@dev.mellanox.co.il>
> >
> > ---
> >
> > I find the following useful.
> > This currently only covers native git protocol. I expect it would
> > be easy to extend this to other protocols, if there's interest.
> > Opinions?
> 
> I think giving this kind of feedback makes a lot of sense, from
> both the "assurance" point of view and also debuggability.
> 
> But please do this only under verbose, or squelch it if "quiet"
> is asked.

Squelching it if quiet is set makes more sense to me. I'll do that.

-- 
MST
Michael S. Tsirkin· May 10, 2007, 09:51 UTC · re: Junio C Hamano · lore

[PATCHv2] connect: display connection progress

Make git notify the user about host resolution/connection attempts. This is useful both as a progress indicator on slow links, and helps reassure the user there are no DNS/firewall problems.

Signed-off-by: Michael S. Tsirkin <mst@dev.mellanox.co.il>
---
Show 15 quoted lines
> > I find the following useful.
> > This currently only covers native git protocol. I expect it would
> > be easy to extend this to other protocols, if there's interest.
> > Opinions?
> 
> Quoting Junio C Hamano <junkio@cox.net>:
> Subject: Re: [PATCH] connect: display connection progress
> 
> "Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
> 
> I think giving this kind of feedback makes a lot of sense, from
> both the "assurance" point of view and also debuggability.
> 
> But please do this only under verbose, or squelch it if "quiet"
> is asked.
Here's an updated patch. Please comment.
Show changes to 6 files +30 −11

builtin-archive.c, cache.h, connect.c, fetch-pack.c, peek-remote.c, send-pack.c

diff --git a/builtin-archive.c b/builtin-archive.c
index 7f4e409..5312e89 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -45,7 +45,7 @@ static int run_remote_archiver(const char *remote, int argc,
 	}
 
 	url = xstrdup(remote);
-	pid = git_connect(fd, url, exec);
+	pid = git_connect(fd, url, exec, NET_QUIET);
 	if (pid < 0)
 		return pid;
 
diff --git a/cache.h b/cache.h
index 8e76152..232faa7 100644
--- a/cache.h
+++ b/cache.h
@@ -462,7 +462,8 @@ struct ref {
 #define REF_HEADS	(1u << 1)
 #define REF_TAGS	(1u << 2)
 
-extern pid_t git_connect(int fd[2], char *url, const char *prog);
+#define NET_QUIET       (1u << 0)
+extern pid_t git_connect(int fd[2], char *url, const char *prog, int flags);
 extern int finish_connect(pid_t pid);
 extern int path_match(const char *path, int nr, char **match);
 extern int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
diff --git a/connect.c b/connect.c
index da89c9c..fd4718a 100644
--- a/connect.c
+++ b/connect.c
@@ -394,7 +394,7 @@ static enum protocol get_protocol(const char *name)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)
 {
 	int sockfd = -1, saved_errno = 0;
 	char *colon, *end;
@@ -425,10 +425,16 @@ static int git_tcp_connect_sock(char *host)
 	hints.ai_socktype = SOCK_STREAM;
 	hints.ai_protocol = IPPROTO_TCP;
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "Looking up %s ... ", host);
+
 	gai = getaddrinfo(host, port, &hints, &ai);
 	if (gai)
 		die("Unable to look up %s (port %s) (%s)", host, port, gai_strerror(gai));
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
+
 	for (ai0 = ai; ai; ai = ai->ai_next) {
 		sockfd = socket(ai->ai_family,
 				ai->ai_socktype, ai->ai_protocol);
@@ -450,6 +456,9 @@ static int git_tcp_connect_sock(char *host)
 	if (sockfd < 0)
 		die("unable to connect a socket (%s)", strerror(saved_errno));
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\n");
+
 	return sockfd;
 }
 
@@ -458,7 +467,7 @@ static int git_tcp_connect_sock(char *host)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host)
+static int git_tcp_connect_sock(char *host, int flags)
 {
 	int sockfd = -1, saved_errno = 0;
 	char *colon, *end;
@@ -485,6 +494,9 @@ static int git_tcp_connect_sock(char *host)
 		port = colon + 1;
 	}
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "Looking up %s ... ", host);
+
 	he = gethostbyname(host);
 	if (!he)
 		die("Unable to look up %s (%s)", host, hstrerror(h_errno));
@@ -497,6 +509,9 @@ static int git_tcp_connect_sock(char *host)
 		nport = se->s_port;
 	}
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
+
 	for (ap = he->h_addr_list; *ap; ap++) {
 		sockfd = socket(he->h_addrtype, SOCK_STREAM, 0);
 		if (sockfd < 0) {
@@ -521,15 +536,18 @@ static int git_tcp_connect_sock(char *host)
 	if (sockfd < 0)
 		die("unable to connect a socket (%s)", strerror(saved_errno));
 
+	if (!(flags & NET_QUIET))
+		fprintf(stderr, "done.\n");
+
 	return sockfd;
 }
 
 #endif /* NO_IPV6 */
 
 
-static void git_tcp_connect(int fd[2], char *host)
+static void git_tcp_connect(int fd[2], char *host, int flags)
 {
-	int sockfd = git_tcp_connect_sock(host);
+	int sockfd = git_tcp_connect_sock(host, flags);
 
 	fd[0] = sockfd;
 	fd[1] = dup(sockfd);
@@ -646,7 +664,7 @@ static void git_proxy_connect(int fd[2], char *host)
  *
  * Does not return a negative value on error; it just dies.
  */
-pid_t git_connect(int fd[2], char *url, const char *prog)
+pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 {
 	char *host, *path = url;
 	char *end;
@@ -719,7 +737,7 @@ pid_t git_connect(int fd[2], char *url, const char *prog)
 		if (git_use_proxy(host))
 			git_proxy_connect(fd, host);
 		else
-			git_tcp_connect(fd, host);
+			git_tcp_connect(fd, host, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended components with a NUL byte.
diff --git a/fetch-pack.c b/fetch-pack.c
index 06f4aec..050b01d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -733,7 +733,7 @@ int main(int argc, char **argv)
 	}
 	if (!dest)
 		usage(fetch_pack_usage);
-	pid = git_connect(fd, dest, uploadpack);
+	pid = git_connect(fd, dest, uploadpack, quiet ? NET_QUIET : 0);
 	if (pid < 0)
 		return 1;
 	if (heads && nr_heads)
diff --git a/peek-remote.c b/peek-remote.c
index 96bfac4..a5e0fc1 100644
--- a/peek-remote.c
+++ b/peek-remote.c
@@ -64,7 +64,7 @@ int main(int argc, char **argv)
 	if (!dest || i != argc - 1)
 		usage(peek_remote_usage);
 
-	pid = git_connect(fd, dest, uploadpack);
+	pid = git_connect(fd, dest, uploadpack, NET_QUIET);
 	if (pid < 0)
 		return 1;
 	ret = peek_remote(fd, flags);
diff --git a/send-pack.c b/send-pack.c
index d5b5162..5d99b25 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -393,7 +393,7 @@ int main(int argc, char **argv)
 		usage(send_pack_usage);
 	verify_remote_names(nr_heads, heads);
 
-	pid = git_connect(fd, dest, receivepack);
+	pid = git_connect(fd, dest, receivepack, verbose ? 0 : NET_QUIET);
 	if (pid < 0)
 		return 1;
 	ret = send_pack(fd[0], fd[1], nr_heads, heads);
-- 
MST
Alex Riesen· May 10, 2007, 11:39 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> -static int git_tcp_connect_sock(char *host)
> +static int git_tcp_connect_sock(char *host, int flags)

There is only one bit of flags ever used. What are the others for? Why use negative logic? What was wrong with plain "int verbose"? What addresses were tried by connect?

Michael S. Tsirkin· May 10, 2007, 12:08 UTC · re: Alex Riesen · lore

Re: [PATCHv2] connect: display connection progress

Show 8 quoted lines
> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >-static int git_tcp_connect_sock(char *host)
> >+static int git_tcp_connect_sock(char *host, int flags)
> 
> There is only one bit of flags ever used. What are the others for?
Hmm, I thought it's easier to read 
git_tcp_connect_sock(host, NET_QUIET)
	than
git_tcp_connect_sock(host, 1)
but maybe that's overdesign.
> Why use negative logic?
> What was wrong with plain "int verbose"?

I want the default to report connections, and -q to silence them. Maybe "int quiet"?

> What addresses were tried by connect?

You are speaking about your patch reporting the IP on failure? I think it makes sense, but it's a separate issue, isn't it?

-- 
MST
Alex Riesen· May 10, 2007, 12:19 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
Show 11 quoted lines
> > Quoting Alex Riesen <raa.lkml@gmail.com>:
> > Subject: Re: [PATCHv2] connect: display connection progress
> >
> > On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> > >-static int git_tcp_connect_sock(char *host)
> > >+static int git_tcp_connect_sock(char *host, int flags)
> >
> > There is only one bit of flags ever used. What are the others for?
>
> Hmm, I thought it's easier to read
> git_tcp_connect_sock(host, NET_QUIET)
It is easier to read. "int flags" isn't easier to understand.
Show 5 quoted lines
> > Why use negative logic?
> > What was wrong with plain "int verbose"?
>
> I want the default to report connections, and -q
> to silence them. Maybe "int quiet"?

It depends. "Quiet" is negative, which automatically makes the logic harder to follow (for humans, at least), and you had to put negations all over git_tcp_connect, exactly because the meaning is exactly the opposite to what you need.

> > What addresses were tried by connect?
>
> You are speaking about your patch reporting the IP on failure?

Yes. Not on failure (not only). Every time an address is tried to connect.

> I think it makes sense, but it's a separate issue, isn't it?

You are just about to make git_tcp_connect verbose, are you not?

Michael S. Tsirkin· May 10, 2007, 12:25 UTC · re: Alex Riesen · lore

Re: [PATCHv2] connect: display connection progress

Show 36 quoted lines
> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >> Quoting Alex Riesen <raa.lkml@gmail.com>:
> >> Subject: Re: [PATCHv2] connect: display connection progress
> >>
> >> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >> >-static int git_tcp_connect_sock(char *host)
> >> >+static int git_tcp_connect_sock(char *host, int flags)
> >>
> >> There is only one bit of flags ever used. What are the others for?
> >
> >Hmm, I thought it's easier to read
> >git_tcp_connect_sock(host, NET_QUIET)
> 
> It is easier to read. "int flags" isn't easier to understand.
> 
> >> Why use negative logic?
> >> What was wrong with plain "int verbose"?
> >
> >I want the default to report connections, and -q
> >to silence them. Maybe "int quiet"?
> 
> It depends. "Quiet" is negative, which automatically
> makes the logic harder to follow (for humans, at least),
> and you had to put negations all over git_tcp_connect,
> exactly because the meaning is exactly the opposite to
> what you need.
> 
> >> What addresses were tried by connect?
> >
> >You are speaking about your patch reporting the IP on failure?
> 
> Yes. Not on failure (not only). Every time an address is tried
> to connect.
Why not only on failure? IP addresses look ugly.
> >I think it makes sense, but it's a separate issue, isn't it?
> 
> You are just about to make git_tcp_connect verbose,
> are you not?

Only if the flag is set. So git-fetch without -q qill be more verbose - but it already spits out a fair amount of data on screen.

-- 
MST
Alex Riesen· May 10, 2007, 13:33 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
Show 8 quoted lines
> > >> What addresses were tried by connect?
> > >
> > >You are speaking about your patch reporting the IP on failure?
> >
> > Yes. Not on failure (not only). Every time an address is tried
> > to connect.
>
> Why not only on failure? IP addresses look ugly.

So you can see DNS problems you wanted to uncover. DNS is all about mapping names to that ugly IP. And DNS _problems_ often manifest themselves by mapping the name to an unexpected IP. Now that's really ugly

Show 7 quoted lines
> > >I think it makes sense, but it's a separate issue, isn't it?
> >
> > You are just about to make git_tcp_connect verbose,
> > are you not?
>
> Only if the flag is set. So git-fetch without -q qill be more verbose -
> but it already spits out a fair amount of data on screen.
And so you added some more? Does not sound logical.

How about cleaning up this (reduce the amount of date on screen) and adding another verbosity level (with your messages and IP) instead?

Michael S. Tsirkin· May 10, 2007, 13:46 UTC · re: Alex Riesen · lore

Re: [PATCHv2] connect: display connection progress

Show 14 quoted lines
> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >> >> What addresses were tried by connect?
> >> >
> >> >You are speaking about your patch reporting the IP on failure?
> >>
> >> Yes. Not on failure (not only). Every time an address is tried
> >> to connect.
> >
> >Why not only on failure? IP addresses look ugly.
> 
> So you can see DNS problems you wanted to uncover.

I really just wanted git to tell me what it's doing, so that I know it's not actually blocked on network, not doing any work.

> DNS is all about mapping names to that ugly IP.

Yes, but so far git port does not seem to be commonly open on random IPs ;).

> And DNS _problems_ often manifest themselves
> by mapping the name to an unexpected IP.
> Now that's really ugly

So, let's print the IP if -v is set? Oh, look, now we'll have NET_QUIET NET_VERBOSE

Show 12 quoted lines
> >> >I think it makes sense, but it's a separate issue, isn't it?
> >>
> >> You are just about to make git_tcp_connect verbose,
> >> are you not?
> >
> >Only if the flag is set. So git-fetch without -q qill be more verbose -
> >but it already spits out a fair amount of data on screen.
> 
> And so you added some more? Does not sound logical.
> 
> How about cleaning up this (reduce the amount of date
> on screen)
Isn't this why we have -q?
> and adding another verbosity level (with your
> messages and IP) instead?
What, yet another flag? Nooooooo
-- 
MST
Alex Riesen· May 10, 2007, 14:16 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
Show 7 quoted lines
> > >Why not only on failure? IP addresses look ugly.
> >
> > So you can see DNS problems you wanted to uncover.
>
> I really just wanted git to tell me what it's doing,
> so that I know it's not actually blocked on network,
> not doing any work.
Aren't you interested in _what_ work is it doing?
> > DNS is all about mapping names to that ugly IP.
>
> Yes, but so far git port does not seem to be commonly open
> on random IPs ;).
Well, it does. It happened.
Show 8 quoted lines
> > And DNS _problems_ often manifest themselves
> > by mapping the name to an unexpected IP.
> > Now that's really ugly
>
> So, let's print the IP if -v is set?
> Oh, look, now we'll have
> NET_QUIET
> NET_VERBOSE

No. All you have is QUIET and !QUIET (or VERBOSE and !VERBOSE which is the same).

> > How about cleaning up this (reduce the amount of date
> > on screen)
>
> Isn't this why we have -q?

Only fetch-pack has -q (and -v, which is confusing to say the least).

Michael S. Tsirkin· May 10, 2007, 14:39 UTC · re: Alex Riesen · lore

Re: [PATCHv2] connect: display connection progress

Show 7 quoted lines
> >> How about cleaning up this (reduce the amount of date
> >> on screen)
> >
> >Isn't this why we have -q?
> 
> Only fetch-pack has -q (and -v, which is confusing to
> say the least).

I find it quite proper to have both. I would expect the following:

- git fetch normally displays progress meter, possibly
  tells me what stage it's in (connecting/downloading ....)
  so I know it's not hung.
- git fetch -q only tells me about errors/exceptional events
  good e.g. for scripts.
- git fetch -v gives a lot of detail useful for debugging
  only used if I see problems and want to debug.
Isn't this what's going on?
So, that's why the "connecting" message belongs in the default setup
(it can hang there for minutes), IP and such technicalia
belong with -v, and -q would only print data on connection error.
 
-- 
MST
Alex Riesen· May 10, 2007, 14:52 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> So, that's why the "connecting" message belongs in the default setup
> (it can hang there for minutes), IP and such technicalia
> belong with -v, and -q would only print data on connection error.

How about a config option? So that people working with repos connected through fast links (say, in local network or even locally on the same system) are not bothered by the "connecting" messages (they're is useless then, local networks usually work).

Michael S. Tsirkin· May 10, 2007, 15:02 UTC · re: Alex Riesen · lore

Re: [PATCHv2] connect: display connection progress

Show 13 quoted lines
> Quoting Alex Riesen <raa.lkml@gmail.com>:
> Subject: Re: [PATCHv2] connect: display connection progress
> 
> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
> >So, that's why the "connecting" message belongs in the default setup
> >(it can hang there for minutes), IP and such technicalia
> >belong with -v, and -q would only print data on connection error.
> 
> How about a config option? So that people working with
> repos connected through fast links (say, in local network or
> even locally on the same system) are not bothered by the
> "connecting" messages (they're is useless then, local networks
> usually work).

Do you really have git servers accessed over a local lan or on local system? I just use ssh in this case, and I think that's the common case ...

I think making it possible to make -q a config option would be useful, though.

We *could* try doing something smart with non-blocking connect + select, and only print the message if it takes > 1 second. Are you sure it's worth the complication?

-- 
MST
Alex Riesen· May 10, 2007, 17:40 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
>
> Do you really have git servers accessed over a local lan or on local system?
Yes and yes.
> I just use ssh in this case, and I think that's the common case ...
not on windows
> We *could* try doing something smart with non-blocking connect + select,
> and only print the message if it takes > 1 second. Are you
> sure it's worth the complication?

I'm not sure this suggestion of yours is worth the complication. Besides, it's hard to get portably

Junio C Hamano· May 10, 2007, 19:29 UTC · re: Michael S. Tsirkin · lore

Re: [PATCHv2] connect: display connection progress

"Michael S. Tsirkin" <mst@dev.mellanox.co.il> writes:
Show 21 quoted lines
>> Quoting Alex Riesen <raa.lkml@gmail.com>:
>> Subject: Re: [PATCHv2] connect: display connection progress
>> 
>> On 5/10/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
>> >-static int git_tcp_connect_sock(char *host)
>> >+static int git_tcp_connect_sock(char *host, int flags)
>> 
>> There is only one bit of flags ever used. What are the others for?
>
> Hmm, I thought it's easier to read 
> git_tcp_connect_sock(host, NET_QUIET)
> 	than
> git_tcp_connect_sock(host, 1)
>
> but maybe that's overdesign.
>
>> Why use negative logic?
>> What was wrong with plain "int verbose"?
>
> I want the default to report connections, and -q
> to silence them. Maybe "int quiet"?

I would really feel this extra verbosity should not be the default. Thanks.

Linus Torvalds· May 10, 2007, 16:05 UTC · re: Alex Riesen · lore

Re: [PATCHv2] connect: display connection progress

On Thu, 10 May 2007, Alex Riesen wrote:
> 
> There is only one bit of flags ever used. What are the others for?

I actually think it tends to be better to have a "flags" field rather than a boolean, even if it only ends up having one flag.

> Why use negative logic?

This one I agree with. Ity would be nicer with CONNECT_VERBOSE than with NET_QUIET, and having the tests be

	if (flags & CONNECT_VERBOSE)
		..
instead.
> What was wrong with plain "int verbose"?

I could see wanting to add flags to do things like disable insecure connections etc, so there's certainly nothing saying that "verbose" is the only valid way to do things.

> What addresses were tried by connect?
That would be _really_ verbose. Maybe a CONNECT_EXTRA_VERBOSE?
		Linus
Alex Riesen· May 10, 2007, 17:38 UTC · re: Linus Torvalds · lore

Re: [PATCHv2] connect: display connection progress

On 5/10/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > What addresses were tried by connect?
>
> That would be _really_ verbose. Maybe a CONNECT_EXTRA_VERBOSE?
>

It's nice to have. That's how I discovered which one of kernel.org addresses is more stable.

Alex Riesen· May 6, 2007, 22:21 UTC · re: Michael S. Tsirkin · lore

Re: [PATCH] connect: display connection progress

Michael S. Tsirkin, Sun, May 06, 2007 21:52:30 +0200:
> Make git notify the user about host resolution/connection attempts.  This
> is useful both as a progress indicator on slow links, and helps reassure the
> user there are no DNS/firewall problems.

If there DNS problems, it is usually useful to show which address was used. And what errors were, exactly (not just errno, but h_errno too). And which one of many, BTW. Maybe this below could be of help:

Show changes to connect.c +36 −1
diff --git a/connect.c b/connect.c
index da89c9c..34c26bb 100644
--- a/connect.c
+++ b/connect.c
@@ -391,6 +391,23 @@ static enum protocol get_protocol(const char *name)
 
 #ifndef NO_IPV6
 
+static const char *ai_name(const struct addrinfo *ai)
+{
+	static char addr[INET_ADDRSTRLEN];
+	if ( AF_INET == ai->ai_family ) {
+		struct sockaddr_in *in;
+		in = (struct sockaddr_in *)ai->ai_addr;
+		inet_ntop(ai->ai_family, &in->sin_addr, addr, sizeof(addr));
+	} else if ( AF_INET6 == ai->ai_family ) {
+		struct sockaddr_in6 *in;
+		in = (struct sockaddr_in6 *)ai->ai_addr;
+		inet_ntop(ai->ai_family, &in->sin6_addr, addr, sizeof(addr));
+	} else {
+		strcpy(addr, "(unknown)");
+	}
+	return addr;
+}
+
 /*
  * Returns a connected socket() fd, or else die()s.
  */
@@ -401,6 +418,7 @@ static int git_tcp_connect_sock(char *host)
 	const char *port = STR(DEFAULT_GIT_PORT);
 	struct addrinfo hints, *ai0, *ai;
 	int gai;
+	int cnt = 0;
 
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
@@ -438,10 +456,17 @@ static int git_tcp_connect_sock(char *host)
 		}
 		if (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			saved_errno = errno;
+			fprintf(stderr, "%s[%d: %s]: net=%s, errno=%s\n",
+				host,
+				cnt,
+				ai_name(ai),
+				hstrerror(h_errno),
+				strerror(saved_errno));
 			close(sockfd);
 			sockfd = -1;
 			continue;
 		}
+		fprintf(stderr, "using %s[%s]\n", host, ai_name(ai));
 		break;
 	}
 
@@ -467,6 +492,7 @@ static int git_tcp_connect_sock(char *host)
 	struct sockaddr_in sa;
 	char **ap;
 	unsigned int nport;
+	int cnt;
 
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
@@ -497,7 +523,7 @@ static int git_tcp_connect_sock(char *host)
 		nport = se->s_port;
 	}
 
-	for (ap = he->h_addr_list; *ap; ap++) {
+	for (cnt = 0, ap = he->h_addr_list; *ap; ap++, cnt++) {
 		sockfd = socket(he->h_addrtype, SOCK_STREAM, 0);
 		if (sockfd < 0) {
 			saved_errno = errno;
@@ -511,10 +537,19 @@ static int git_tcp_connect_sock(char *host)
 
 		if (connect(sockfd, (struct sockaddr *)&sa, sizeof sa) < 0) {
 			saved_errno = errno;
+			fprintf(stderr, "%s[%d: %s]: net=%s, errno=%s\n",
+				host,
+				cnt,
+				inet_ntoa(*(struct in_addr *)&sa.sin_addr),
+				hstrerror(h_errno),
+				strerror(saved_errno));
 			close(sockfd);
 			sockfd = -1;
 			continue;
 		}
+		fprintf(stderr, "using %s[%s]\n",
+			host,
+			inet_ntoa(*(struct in_addr *)&sa.sin_addr));
 		break;
 	}
 
Michael S. Tsirkin· May 7, 2007, 04:54 UTC · re: Alex Riesen · lore

Re: [PATCH] connect: display connection progress

Show 19 quoted lines
> @@ -511,10 +537,19 @@ static int git_tcp_connect_sock(char *host)
>  
>  		if (connect(sockfd, (struct sockaddr *)&sa, sizeof sa) < 0) {
>  			saved_errno = errno;
> +			fprintf(stderr, "%s[%d: %s]: net=%s, errno=%s\n",
> +				host,
> +				cnt,
> +				inet_ntoa(*(struct in_addr *)&sa.sin_addr),
> +				hstrerror(h_errno),
> +				strerror(saved_errno));
>  			close(sockfd);
>  			sockfd = -1;
>  			continue;
>  		}
> +		fprintf(stderr, "using %s[%s]\n",
> +			host,
> +			inet_ntoa(*(struct in_addr *)&sa.sin_addr));
>  		break;
>  	}
My manual says:
	The inet_ntoa() function shall convert the Internet host address
	specified by in to a string in the Internet standard dot notation.
does it work for IPv6?
-- 
MST
Alex Riesen· May 7, 2007, 07:51 UTC · re: Michael S. Tsirkin · lore

Re: [PATCH] connect: display connection progress

On 5/7/07, Michael S. Tsirkin <mst@dev.mellanox.co.il> wrote:
Show 5 quoted lines
> My manual says:
>         The inet_ntoa() function shall convert the Internet host address
>         specified by in to a string in the Internet standard dot notation.
>
> does it work for IPv6?
does it have to if NO_IPV6 is set?

← back to recent threads