[1/4] resolv: improve comments about nserv and nservall

Message ID 1402673533-13243-2-git-send-email-aurelien@aurel32.net
State Committed
Headers

Commit Message

Aurelien Jarno June 13, 2014, 3:32 p.m. UTC
  The current comments concerning nserv and nservall are not really clear
and lead to confusion when reviewing an already complex code. Improve
them, there real meaning have been confirmed by a code analysis.

2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>

	* resolv/res_init.c (__res_vinit): Improve comments
	about nserv and nservall.
  

Comments

Siddhesh Poyarekar June 18, 2014, 9:12 a.m. UTC | #1
On Fri, Jun 13, 2014 at 05:32:10PM +0200, Aurelien Jarno wrote:
> The current comments concerning nserv and nservall are not really clear
> and lead to confusion when reviewing an already complex code. Improve
> them, there real meaning have been confirmed by a code analysis.
> 
> 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
> 
> 	* resolv/res_init.c (__res_vinit): Improve comments
> 	about nserv and nservall.

I already acked this.

Siddhesh

> 
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index ea133f8..bdec4d9 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -153,9 +153,9 @@ __res_vinit(res_state statp, int preinit) {
>  	char *cp, **pp;
>  	int n;
>  	char buf[BUFSIZ];
> -	int nserv = 0;    /* number of nameserver records read from file */
> +	int nserv = 0;    /* number of IPv4 nameservers read from file */
>  #ifdef _LIBC
> -	int nservall = 0; /* number of NS records read, nserv IPv4 only */
> +	int nservall = 0; /* number of (IPv4 + IPV6) nameservers read from file */
>  #endif
>  	int haveenv = 0;
>  	int havesearch = 0;
> -- 
> 2.0.0
>
  
Aurelien Jarno June 18, 2014, 4:28 p.m. UTC | #2
On Wed, Jun 18, 2014 at 02:42:25PM +0530, Siddhesh Poyarekar wrote:
> On Fri, Jun 13, 2014 at 05:32:10PM +0200, Aurelien Jarno wrote:
> > The current comments concerning nserv and nservall are not really clear
> > and lead to confusion when reviewing an already complex code. Improve
> > them, there real meaning have been confirmed by a code analysis.
> > 
> > 2014-06-13  Aurelien Jarno  <aurelien@aurel32.net>
> > 
> > 	* resolv/res_init.c (__res_vinit): Improve comments
> > 	about nserv and nservall.
> 
> I already acked this.
> 

Thanks for your ack. My plan is to commit the 4 patches at the same
time, as they are linked. I am waiting for the review of the 3 other
patches, unless I am mistaken and that this ack is for the whole series.
  
Siddhesh Poyarekar June 19, 2014, 9:51 a.m. UTC | #3
On Wed, Jun 18, 2014 at 06:28:31PM +0200, Aurelien Jarno wrote:
> Thanks for your ack. My plan is to commit the 4 patches at the same
> time, as they are linked. I am waiting for the review of the 3 other
> patches, unless I am mistaken and that this ack is for the whole series.

They're linked, but not dependent, but it's your personal preference I
guess.  My concern is that it breaks the patchwork workflow, since
you're resent the acked patch again.  I haven't looked at the other
patches to see if they're changed, but if they're not and your
intention was to just ping the patches, then I'd suggest using the
method everyone else is using, which is to add [ping] to the subject
line in reply to your original submission instead of resending the
patch, since the latter adds an additional patch to review in
patchwork.

That, or help maintain the patchwork reviews, and I'd really be glad
if you choose the latter :)

Siddhesh
  
Aurelien Jarno June 19, 2014, 3:53 p.m. UTC | #4
On Thu, Jun 19, 2014 at 03:21:42PM +0530, Siddhesh Poyarekar wrote:
> On Wed, Jun 18, 2014 at 06:28:31PM +0200, Aurelien Jarno wrote:
> > Thanks for your ack. My plan is to commit the 4 patches at the same
> > time, as they are linked. I am waiting for the review of the 3 other
> > patches, unless I am mistaken and that this ack is for the whole series.
> 
> They're linked, but not dependent, but it's your personal preference I
> guess.  My concern is that it breaks the patchwork workflow, since
> you're resent the acked patch again.  I haven't looked at the other
> patches to see if they're changed, but if they're not and your
> intention was to just ping the patches, then I'd suggest using the
> method everyone else is using, which is to add [ping] to the subject
> line in reply to your original submission instead of resending the
> patch, since the latter adds an additional patch to review in
> patchwork.

One of the patch has changed, all the others got improved description
and a changelog entry. But more importantly the first version was
clearly an RFC, one of the main change between the two is that I am now
convinced it's the right thing to do.

> That, or help maintain the patchwork reviews, and I'd really be glad
> if you choose the latter :)

Yes, in that case I could simply flag the patch as superseded in
patchworks. I have just created an account, so that I can do that next
time.
  
Siddhesh Poyarekar June 19, 2014, 4:13 p.m. UTC | #5
On Thu, Jun 19, 2014 at 05:53:37PM +0200, Aurelien Jarno wrote:
> Yes, in that case I could simply flag the patch as superseded in
> patchworks. I have just created an account, so that I can do that next
> time. 

... and I have added you as a maintainer so that you can change state
of patches you post and review.

Thanks!
Siddhesh
  
H.J. Lu Jan. 6, 2015, 5:01 p.m. UTC | #6
On Thu, Jun 19, 2014 at 9:13 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Thu, Jun 19, 2014 at 05:53:37PM +0200, Aurelien Jarno wrote:
>> Yes, in that case I could simply flag the patch as superseded in
>> patchworks. I have just created an account, so that I can do that next
>> time.
>
> ... and I have added you as a maintainer so that you can change state
> of patches you post and review.
>

I checked it in.
  

Patch

diff --git a/resolv/res_init.c b/resolv/res_init.c
index ea133f8..bdec4d9 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -153,9 +153,9 @@  __res_vinit(res_state statp, int preinit) {
 	char *cp, **pp;
 	int n;
 	char buf[BUFSIZ];
-	int nserv = 0;    /* number of nameserver records read from file */
+	int nserv = 0;    /* number of IPv4 nameservers read from file */
 #ifdef _LIBC
-	int nservall = 0; /* number of NS records read, nserv IPv4 only */
+	int nservall = 0; /* number of (IPv4 + IPV6) nameservers read from file */
 #endif
 	int haveenv = 0;
 	int havesearch = 0;