[PATCH v2] ws2_32: Drop dependencies on system getprotoby(name|number) functions
Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> --- v2: - Keep original protocol table format - Implement getprotobynumber using binary search --- configure.ac | 2 - dlls/ws2_32/socket.c | 76 ++++++++------------------- dlls/ws2_32/tests/protocol.c | 99 ++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 57 deletions(-) diff --git a/configure.ac b/configure.ac index 4829648c3a..9002ce8d60 100644 --- a/configure.ac +++ b/configure.ac @@ -2245,8 +2245,6 @@ AC_CHECK_FUNCS(\ getaddrinfo \ getnameinfo \ getnetbyname \ - getprotobyname \ - getprotobynumber \ getservbyport \ ) diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 69fbb37af2..a860258beb 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -6507,58 +6507,26 @@ struct WS_hostent* WINAPI WS_gethostbyname(const char* name) static const struct { int prot; const char *names[3]; } protocols[] = { + /* keep this list sorted by number for binary search */ { 0, { "ip", "IP" }}, { 1, { "icmp", "ICMP" }}, - { 2, { "igmp", "IGMP" }}, { 3, { "ggp", "GGP" }}, { 6, { "tcp", "TCP" }}, { 8, { "egp", "EGP" }}, - { 9, { "igp", "IGP" }}, { 12, { "pup", "PUP" }}, { 17, { "udp", "UDP" }}, { 20, { "hmp", "HMP" }}, { 22, { "xns-idp", "XNS-IDP" }}, { 27, { "rdp", "RDP" }}, - { 29, { "iso-tp4", "ISO-TP4" }}, - { 33, { "dccp", "DCCP" }}, - { 36, { "xtp", "XTP" }}, - { 37, { "ddp", "DDP" }}, - { 38, { "idpr-cmtp", "IDPR-CMTP" }}, { 41, { "ipv6", "IPv6" }}, { 43, { "ipv6-route", "IPv6-Route" }}, { 44, { "ipv6-frag", "IPv6-Frag" }}, - { 45, { "idrp", "IDRP" }}, - { 46, { "rsvp", "RSVP" }}, - { 47, { "gre", "GRE" }}, { 50, { "esp", "ESP" }}, { 51, { "ah", "AH" }}, - { 57, { "skip", "SKIP" }}, { 58, { "ipv6-icmp", "IPv6-ICMP" }}, { 59, { "ipv6-nonxt", "IPv6-NoNxt" }}, { 60, { "ipv6-opts", "IPv6-Opts" }}, { 66, { "rvd", "RVD" }}, - { 73, { "rspf", "RSPF" }}, - { 81, { "vmtp", "VMTP" }}, - { 88, { "eigrp", "EIGRP" }}, - { 89, { "ospf", "OSPFIGP" }}, - { 93, { "ax.25", "AX.25" }}, - { 94, { "ipip", "IPIP" }}, - { 97, { "etherip", "ETHERIP" }}, - { 98, { "encap", "ENCAP" }}, - { 103, { "pim", "PIM" }}, - { 108, { "ipcomp", "IPCOMP" }}, - { 112, { "vrrp", "VRRP" }}, - { 115, { "l2tp", "L2TP" }}, - { 124, { "isis", "ISIS" }}, - { 132, { "sctp", "SCTP" }}, - { 133, { "fc", "FC" }}, - { 135, { "mobility-header", "Mobility-Header" }}, - { 136, { "udplite", "UDPLite" }}, - { 137, { "mpls-in-ip", "MPLS-in-IP" }}, - { 139, { "hip", "HIP" }}, - { 140, { "shim6", "Shim6" }}, - { 141, { "wesp", "WESP" }}, - { 142, { "rohc", "ROHC" }}, }; /*********************************************************************** @@ -6567,24 +6535,18 @@ static const struct { int prot; const char *names[3]; } protocols[] = struct WS_protoent* WINAPI WS_getprotobyname(const char* name) { struct WS_protoent* retval = NULL; -#ifdef HAVE_GETPROTOBYNAME - struct protoent* proto; - EnterCriticalSection( &csWSgetXXXbyYYY ); - if( (proto = getprotobyname(name)) != NULL ) - retval = WS_create_pe( proto->p_name, proto->p_aliases, proto->p_proto ); - LeaveCriticalSection( &csWSgetXXXbyYYY ); -#endif - if (!retval) + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(protocols); i++) { - unsigned int i; - for (i = 0; i < ARRAY_SIZE(protocols); i++) + if (_strnicmp( protocols[i].names[0], name, -1 ) == 0) { - if (_strnicmp( protocols[i].names[0], name, -1 )) continue; retval = WS_create_pe( protocols[i].names[0], (char **)protocols[i].names + 1, protocols[i].prot ); break; } } + if (!retval) { WARN( "protocol %s not found\n", debugstr_a(name) ); @@ -6601,24 +6563,28 @@ struct WS_protoent* WINAPI WS_getprotobyname(const char* name) struct WS_protoent* WINAPI WS_getprotobynumber(int number) { struct WS_protoent* retval = NULL; -#ifdef HAVE_GETPROTOBYNUMBER - struct protoent* proto; - EnterCriticalSection( &csWSgetXXXbyYYY ); - if( (proto = getprotobynumber(number)) != NULL ) - retval = WS_create_pe( proto->p_name, proto->p_aliases, proto->p_proto ); - LeaveCriticalSection( &csWSgetXXXbyYYY ); -#endif - if (!retval) + int start = 0, end = ARRAY_SIZE(protocols) - 1, i; + + do { - unsigned int i; - for (i = 0; i < ARRAY_SIZE(protocols); i++) + i = (start + end) / 2; + if (number < protocols[i].prot) + { + end = i - 1; + } + else if (number > protocols[i].prot) + { + start = i + 1; + } + else { - if (protocols[i].prot != number) continue; retval = WS_create_pe( protocols[i].names[0], (char **)protocols[i].names + 1, protocols[i].prot ); break; } } + while (start <= end); + if (!retval) { WARN( "protocol %d not found\n", number ); diff --git a/dlls/ws2_32/tests/protocol.c b/dlls/ws2_32/tests/protocol.c index 99bd1373a9..4cc7770dad 100644 --- a/dlls/ws2_32/tests/protocol.c +++ b/dlls/ws2_32/tests/protocol.c @@ -192,6 +192,103 @@ static void test_WSAEnumProtocolsW(void) } } +struct protocol +{ + int prot; + const char *names[3]; + BOOL missing_from_xp; +}; + +static const struct protocol protocols[] = +{ + /* keep this list sorted by number for intelligent linear search */ + { 0, { "ip", "IP" }}, + { 1, { "icmp", "ICMP" }}, + { 3, { "ggp", "GGP" }}, + { 6, { "tcp", "TCP" }}, + { 8, { "egp", "EGP" }}, + { 12, { "pup", "PUP" }}, + { 17, { "udp", "UDP" }}, + { 20, { "hmp", "HMP" }}, + { 22, { "xns-idp", "XNS-IDP" }}, + { 27, { "rdp", "RDP" }}, + { 41, { "ipv6", "IPv6" }, TRUE}, + { 43, { "ipv6-route", "IPv6-Route" }, TRUE}, + { 44, { "ipv6-frag", "IPv6-Frag" }, TRUE}, + { 50, { "esp", "ESP" }}, + { 51, { "ah", "AH" }}, + { 58, { "ipv6-icmp", "IPv6-ICMP" }, TRUE}, + { 59, { "ipv6-nonxt", "IPv6-NoNxt" }, TRUE}, + { 60, { "ipv6-opts", "IPv6-Opts" }, TRUE}, + { 66, { "rvd", "RVD" }}, + { } +}; + +void test_getprotobyname(void) +{ + struct protoent *ent; + char all_caps_name[16]; + int i, j; + + for (i = 0; i < ARRAY_SIZE(protocols) - 1; i++) + { + for (j = 0; protocols[i].names[j]; j++) + { + ent = getprotobyname(protocols[i].names[j]); + ok((ent && ent->p_proto == protocols[i].prot) || broken(!ent && protocols[i].missing_from_xp), + "Expected %s to be protocol number %d, got %d\n", + wine_dbgstr_a(protocols[i].names[j]), protocols[i].prot, ent ? ent->p_proto : -1); + } + + if (protocols[i].names[0]) + { + for (j = 0; protocols[i].names[0][j]; j++) + all_caps_name[j] = toupper(protocols[i].names[0][j]); + all_caps_name[j] = 0; + ent = getprotobyname(all_caps_name); + ok((ent && ent->p_proto == protocols[i].prot) || broken(!ent && protocols[i].missing_from_xp), + "Expected %s to be protocol number %d, got %d\n", + wine_dbgstr_a(all_caps_name), protocols[i].prot, ent ? ent->p_proto : -1); + } + } +} + +void test_getprotobynumber(void) +{ + struct protoent *ent; + const struct protocol *ref = &protocols[0]; + int i, j; + + for (i = -1; i <= 256; i++) + { + ent = getprotobynumber(i); + + if (i != ref->prot) + { + ok(!ent, + "Expected protocol number %d to be undefined, got %s\n", + i, wine_dbgstr_a(ent ? ent->p_name : NULL)); + continue; + } + + ok((ent && ent->p_name && strcmp(ent->p_name, ref->names[0]) == 0) || + broken(!ent && ref->missing_from_xp), + "Expected protocol number %d to be %s, got %s\n", + i, ref->names[0], wine_dbgstr_a(ent ? ent->p_name : NULL)); + + for (j = 0; ref->names[1+j]; j++) + { + ok((ent && ent->p_aliases && ent->p_aliases[j] && + strcmp(ent->p_aliases[j], ref->names[1+j]) == 0) || + broken(!ent && ref->missing_from_xp), + "Expected protocol number %d alias %d to be %s, got %s\n", + i, j, ref->names[0], wine_dbgstr_a(ent && ent->p_aliases ? ent->p_aliases[j] : NULL)); + } + + ref++; + } +} + START_TEST( protocol ) { WSADATA data; @@ -201,4 +298,6 @@ START_TEST( protocol ) test_WSAEnumProtocolsA(); test_WSAEnumProtocolsW(); + test_getprotobyname(); + test_getprotobynumber(); } -- 2.27.0
On 7/14/20 12:35 AM, Alex Henrie wrote:
Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> --- v2: - Keep original protocol table format - Implement getprotobynumber using binary search --- configure.ac | 2 - dlls/ws2_32/socket.c | 76 ++++++++------------------- dlls/ws2_32/tests/protocol.c | 99 ++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 57 deletions(-)
diff --git a/configure.ac b/configure.ac index 4829648c3a..9002ce8d60 100644 --- a/configure.ac +++ b/configure.ac @@ -2245,8 +2245,6 @@ AC_CHECK_FUNCS(\ getaddrinfo \ getnameinfo \ getnetbyname \ - getprotobyname \ - getprotobynumber \ getservbyport \ )
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 69fbb37af2..a860258beb 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -6507,58 +6507,26 @@ struct WS_hostent* WINAPI WS_gethostbyname(const char* name)
static const struct { int prot; const char *names[3]; } protocols[] = { + /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
{ 0, { "ip", "IP" }}, { 1, { "icmp", "ICMP" }}, - { 2, { "igmp", "IGMP" }}, { 3, { "ggp", "GGP" }}, { 6, { "tcp", "TCP" }}, { 8, { "egp", "EGP" }}, - { 9, { "igp", "IGP" }}, { 12, { "pup", "PUP" }}, { 17, { "udp", "UDP" }}, { 20, { "hmp", "HMP" }}, { 22, { "xns-idp", "XNS-IDP" }}, { 27, { "rdp", "RDP" }}, - { 29, { "iso-tp4", "ISO-TP4" }}, - { 33, { "dccp", "DCCP" }}, - { 36, { "xtp", "XTP" }}, - { 37, { "ddp", "DDP" }}, - { 38, { "idpr-cmtp", "IDPR-CMTP" }}, { 41, { "ipv6", "IPv6" }}, { 43, { "ipv6-route", "IPv6-Route" }}, { 44, { "ipv6-frag", "IPv6-Frag" }}, - { 45, { "idrp", "IDRP" }}, - { 46, { "rsvp", "RSVP" }}, - { 47, { "gre", "GRE" }}, { 50, { "esp", "ESP" }}, { 51, { "ah", "AH" }}, - { 57, { "skip", "SKIP" }}, { 58, { "ipv6-icmp", "IPv6-ICMP" }}, { 59, { "ipv6-nonxt", "IPv6-NoNxt" }}, { 60, { "ipv6-opts", "IPv6-Opts" }}, { 66, { "rvd", "RVD" }}, - { 73, { "rspf", "RSPF" }}, - { 81, { "vmtp", "VMTP" }}, - { 88, { "eigrp", "EIGRP" }}, - { 89, { "ospf", "OSPFIGP" }}, - { 93, { "ax.25", "AX.25" }}, - { 94, { "ipip", "IPIP" }}, - { 97, { "etherip", "ETHERIP" }}, - { 98, { "encap", "ENCAP" }}, - { 103, { "pim", "PIM" }}, - { 108, { "ipcomp", "IPCOMP" }}, - { 112, { "vrrp", "VRRP" }}, - { 115, { "l2tp", "L2TP" }}, - { 124, { "isis", "ISIS" }}, - { 132, { "sctp", "SCTP" }}, - { 133, { "fc", "FC" }}, - { 135, { "mobility-header", "Mobility-Header" }}, - { 136, { "udplite", "UDPLite" }}, - { 137, { "mpls-in-ip", "MPLS-in-IP" }}, - { 139, { "hip", "HIP" }}, - { 140, { "shim6", "Shim6" }}, - { 141, { "wesp", "WESP" }}, - { 142, { "rohc", "ROHC" }}, };
/*********************************************************************** @@ -6567,24 +6535,18 @@ static const struct { int prot; const char *names[3]; } protocols[] = struct WS_protoent* WINAPI WS_getprotobyname(const char* name) { struct WS_protoent* retval = NULL; -#ifdef HAVE_GETPROTOBYNAME - struct protoent* proto; - EnterCriticalSection( &csWSgetXXXbyYYY ); - if( (proto = getprotobyname(name)) != NULL ) - retval = WS_create_pe( proto->p_name, proto->p_aliases, proto->p_proto ); - LeaveCriticalSection( &csWSgetXXXbyYYY ); -#endif - if (!retval) + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(protocols); i++) { - unsigned int i; - for (i = 0; i < ARRAY_SIZE(protocols); i++) + if (_strnicmp( protocols[i].names[0], name, -1 ) == 0) { - if (_strnicmp( protocols[i].names[0], name, -1 )) continue; retval = WS_create_pe( protocols[i].names[0], (char **)protocols[i].names + 1, protocols[i].prot ); break; } } + if (!retval) { WARN( "protocol %s not found\n", debugstr_a(name) ); @@ -6601,24 +6563,28 @@ struct WS_protoent* WINAPI WS_getprotobyname(const char* name) struct WS_protoent* WINAPI WS_getprotobynumber(int number) { struct WS_protoent* retval = NULL; -#ifdef HAVE_GETPROTOBYNUMBER - struct protoent* proto; - EnterCriticalSection( &csWSgetXXXbyYYY ); - if( (proto = getprotobynumber(number)) != NULL ) - retval = WS_create_pe( proto->p_name, proto->p_aliases, proto->p_proto ); - LeaveCriticalSection( &csWSgetXXXbyYYY ); -#endif - if (!retval) + int start = 0, end = ARRAY_SIZE(protocols) - 1, i; + + do { - unsigned int i; - for (i = 0; i < ARRAY_SIZE(protocols); i++) + i = (start + end) / 2; + if (number < protocols[i].prot) + { + end = i - 1; + } + else if (number > protocols[i].prot) + { + start = i + 1; + } + else { - if (protocols[i].prot != number) continue; retval = WS_create_pe( protocols[i].names[0], (char **)protocols[i].names + 1, protocols[i].prot ); break; } } + while (start <= end); +
If binary search is worth implementing, can we just use bsearch() to do it?
if (!retval) { WARN( "protocol %d not found\n", number ); diff --git a/dlls/ws2_32/tests/protocol.c b/dlls/ws2_32/tests/protocol.c index 99bd1373a9..4cc7770dad 100644 --- a/dlls/ws2_32/tests/protocol.c +++ b/dlls/ws2_32/tests/protocol.c @@ -192,6 +192,103 @@ static void test_WSAEnumProtocolsW(void) } }
+struct protocol +{ + int prot; + const char *names[3];
I guess this was copied from the implementation, but surely this can just be names[2]?
+ BOOL missing_from_xp; +}; + +static const struct protocol protocols[] = +{ + /* keep this list sorted by number for intelligent linear search */ + { 0, { "ip", "IP" }}, + { 1, { "icmp", "ICMP" }}, + { 3, { "ggp", "GGP" }}, + { 6, { "tcp", "TCP" }}, + { 8, { "egp", "EGP" }}, + { 12, { "pup", "PUP" }}, + { 17, { "udp", "UDP" }}, + { 20, { "hmp", "HMP" }}, + { 22, { "xns-idp", "XNS-IDP" }}, + { 27, { "rdp", "RDP" }}, + { 41, { "ipv6", "IPv6" }, TRUE}, + { 43, { "ipv6-route", "IPv6-Route" }, TRUE}, + { 44, { "ipv6-frag", "IPv6-Frag" }, TRUE}, + { 50, { "esp", "ESP" }}, + { 51, { "ah", "AH" }}, + { 58, { "ipv6-icmp", "IPv6-ICMP" }, TRUE}, + { 59, { "ipv6-nonxt", "IPv6-NoNxt" }, TRUE}, + { 60, { "ipv6-opts", "IPv6-Opts" }, TRUE}, + { 66, { "rvd", "RVD" }}, + { }
If you're using ARRAY_SIZE, does this need to be null-terminated?
+}; + +void test_getprotobyname(void) +{ + struct protoent *ent; + char all_caps_name[16]; + int i, j; + + for (i = 0; i < ARRAY_SIZE(protocols) - 1; i++) + { + for (j = 0; protocols[i].names[j]; j++) + { + ent = getprotobyname(protocols[i].names[j]); + ok((ent && ent->p_proto == protocols[i].prot) || broken(!ent && protocols[i].missing_from_xp), + "Expected %s to be protocol number %d, got %d\n", + wine_dbgstr_a(protocols[i].names[j]), protocols[i].prot, ent ? ent->p_proto : -1); + } + + if (protocols[i].names[0])
Isn't this condition always true?
+ { + for (j = 0; protocols[i].names[0][j]; j++) + all_caps_name[j] = toupper(protocols[i].names[0][j]); + all_caps_name[j] = 0; + ent = getprotobyname(all_caps_name); + ok((ent && ent->p_proto == protocols[i].prot) || broken(!ent && protocols[i].missing_from_xp), + "Expected %s to be protocol number %d, got %d\n", + wine_dbgstr_a(all_caps_name), protocols[i].prot, ent ? ent->p_proto : -1); + } + } +}
+ +void test_getprotobynumber(void) +{ + struct protoent *ent; + const struct protocol *ref = &protocols[0]; + int i, j; + + for (i = -1; i <= 256; i++) + { + ent = getprotobynumber(i); + + if (i != ref->prot) + { + ok(!ent,
This seems like an odd place for a line break.
+ "Expected protocol number %d to be undefined, got %s\n", + i, wine_dbgstr_a(ent ? ent->p_name : NULL)); + continue; + } + + ok((ent && ent->p_name && strcmp(ent->p_name, ref->names[0]) == 0) || + broken(!ent && ref->missing_from_xp), + "Expected protocol number %d to be %s, got %s\n", + i, ref->names[0], wine_dbgstr_a(ent ? ent->p_name : NULL)); + + for (j = 0; ref->names[1+j]; j++) + { + ok((ent && ent->p_aliases && ent->p_aliases[j] && + strcmp(ent->p_aliases[j], ref->names[1+j]) == 0) || + broken(!ent && ref->missing_from_xp), + "Expected protocol number %d alias %d to be %s, got %s\n", + i, j, ref->names[0], wine_dbgstr_a(ent && ent->p_aliases ? ent->p_aliases[j] : NULL)); + } + + ref++; + }
I guess this test structure isn't too confusing on examination, but it might be clearer just to have a semi-reimplementation of getprotobynumber() in the tests, to which you compare the actual data.
+} + START_TEST( protocol ) { WSADATA data; @@ -201,4 +298,6 @@ START_TEST( protocol )
test_WSAEnumProtocolsA(); test_WSAEnumProtocolsW(); + test_getprotobyname(); + test_getprotobynumber(); }
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
Why? I have to change the implementation of getprotobynumber anyway, and I don't want to cause a performance regression, so it seems that I might as well put in the binary search at the same time.
If binary search is worth implementing, can we just use bsearch() to do it?
Probably yeah. I'll give that a try.
+ const char *names[3];
I guess this was copied from the implementation, but surely this can just be names[2]?
The NULL name at the end is used as a sentinel in the tests, the same as in the implementation: for (j = 0; protocols[i].names[0][j]; j++)
+ { }
If you're using ARRAY_SIZE, does this need to be null-terminated?
Yes, it's used as a sentinel in the tests: if (i != ref->prot)
+ if (protocols[i].names[0])
Isn't this condition always true?
Yes it is. I meant to remove this condition in v2. I will fix that in v3. Great catch!
+ ok(!ent,
This seems like an odd place for a line break.
It's meant to mirror the format of the other ok statements in test_getprotobyname and test_getprotobynumber. But it's just a style thing; I'll delete the line break here if more people say that they don't like it.
I guess this test structure isn't too confusing on examination, but it might be clearer just to have a semi-reimplementation of getprotobynumber() in the tests, to which you compare the actual data.
The test is already a semi-reimplementation of getprotobynumber: It has a copy of the protocol table that is identical to the implementation's. How is what you're suggesting different? -Alex
On Tue, Jul 14, 2020 at 9:59 AM Alex Henrie <alexhenrie24(a)gmail.com> wrote:
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ const char *names[3];
I guess this was copied from the implementation, but surely this can just be names[2]?
The NULL name at the end is used as a sentinel in the tests, the same as in the implementation:
for (j = 0; protocols[i].names[0][j]; j++)
Oops, I copied the wrong line: for (j = 0; ref->names[1+j]; j++) -Alex
On 7/14/20 10:59 AM, Alex Henrie wrote:
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
Why? I have to change the implementation of getprotobynumber anyway, and I don't want to cause a performance regression, so it seems that I might as well put in the binary search at the same time.
In that case the patch converting to binary search could be put first. I also have some doubts that the table is large enough for binary search to make a difference.
If binary search is worth implementing, can we just use bsearch() to do it?
Probably yeah. I'll give that a try.
+ const char *names[3];
I guess this was copied from the implementation, but surely this can just be names[2]?
The NULL name at the end is used as a sentinel in the tests, the same as in the implementation:
for (j = 0; protocols[i].names[0][j]; j++)
+ { }
If you're using ARRAY_SIZE, does this need to be null-terminated?
Yes, it's used as a sentinel in the tests:
if (i != ref->prot)
+ if (protocols[i].names[0])
Isn't this condition always true?
Yes it is. I meant to remove this condition in v2. I will fix that in v3. Great catch!
+ ok(!ent,
This seems like an odd place for a line break.
It's meant to mirror the format of the other ok statements in test_getprotobyname and test_getprotobynumber. But it's just a style thing; I'll delete the line break here if more people say that they don't like it.
I think the point of line breaks is just to avoid overly long lines, not to put every parameter on its own line (à la Microsoft...) There's no danger of that here.
I guess this test structure isn't too confusing on examination, but it might be clearer just to have a semi-reimplementation of getprotobynumber() in the tests, to which you compare the actual data.
The test is already a semi-reimplementation of getprotobynumber: It has a copy of the protocol table that is identical to the implementation's. How is what you're suggesting different?
I mean introduce a helper function that looks up the table entry from the number, similarly to what getprotobynumber() does.
-Alex
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
Why? I have to change the implementation of getprotobynumber anyway, and I don't want to cause a performance regression, so it seems that I might as well put in the binary search at the same time.
[...]
It's meant to mirror the format of the other ok statements in test_getprotobyname and test_getprotobynumber. But it's just a style thing; I'll delete the line break here if more people say that they don't like it.
In these cases and in general, please accept criticism and make the requested changes, without requesting input from more people. If someone reviewed your patch and told you that some things need changing, it's safe to assume that you should make the changes and resubmit. -- Alexandre Julliard julliard(a)winehq.org
On Tue, Jul 14, 2020 at 10:25 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
Why? I have to change the implementation of getprotobynumber anyway, and I don't want to cause a performance regression, so it seems that I might as well put in the binary search at the same time.
[...]
It's meant to mirror the format of the other ok statements in test_getprotobyname and test_getprotobynumber. But it's just a style thing; I'll delete the line break here if more people say that they don't like it.
In these cases and in general, please accept criticism and make the requested changes, without requesting input from more people. If someone reviewed your patch and told you that some things need changing, it's safe to assume that you should make the changes and resubmit.
Hi Alexandre, thank you for joining the discussion :-) I am happy to make these two changes. I think I should mention that it's hard for me to not feel a little picked on in situations like this. If the situation were reversed and it were me giving my opinion on Zeb's code style, would the same advice apply to him? -Alex
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Tue, Jul 14, 2020 at 10:25 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
Why? I have to change the implementation of getprotobynumber anyway, and I don't want to cause a performance regression, so it seems that I might as well put in the binary search at the same time.
[...]
It's meant to mirror the format of the other ok statements in test_getprotobyname and test_getprotobynumber. But it's just a style thing; I'll delete the line break here if more people say that they don't like it.
In these cases and in general, please accept criticism and make the requested changes, without requesting input from more people. If someone reviewed your patch and told you that some things need changing, it's safe to assume that you should make the changes and resubmit.
Hi Alexandre, thank you for joining the discussion :-) I am happy to make these two changes.
I think I should mention that it's hard for me to not feel a little picked on in situations like this. If the situation were reversed and it were me giving my opinion on Zeb's code style, would the same advice apply to him?
It seems that you have a tendency to feel picked on, no matter who does the review, but I think that's a mistake. Nobody is trying to pick on you, they are only trying to help you get your patches in. You should try to see the feedback as something positive. And if you look at Zeb's commit stats, you will realize that he probably knows a thing or two about getting patches in, and that it would be smart to take his advice. -- Alexandre Julliard julliard(a)winehq.org
On Tue, Jul 14, 2020 at 1:58 PM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Tue, Jul 14, 2020 at 10:25 AM Alexandre Julliard <julliard(a)winehq.org> wrote:
Alex Henrie <alexhenrie24(a)gmail.com> writes:
On Tue, Jul 14, 2020 at 9:41 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
+ /* keep this list sorted by number for binary search */
This seems to me like it should be a separate patch.
Why? I have to change the implementation of getprotobynumber anyway, and I don't want to cause a performance regression, so it seems that I might as well put in the binary search at the same time.
[...]
It's meant to mirror the format of the other ok statements in test_getprotobyname and test_getprotobynumber. But it's just a style thing; I'll delete the line break here if more people say that they don't like it.
In these cases and in general, please accept criticism and make the requested changes, without requesting input from more people. If someone reviewed your patch and told you that some things need changing, it's safe to assume that you should make the changes and resubmit.
Hi Alexandre, thank you for joining the discussion :-) I am happy to make these two changes.
I think I should mention that it's hard for me to not feel a little picked on in situations like this. If the situation were reversed and it were me giving my opinion on Zeb's code style, would the same advice apply to him?
It seems that you have a tendency to feel picked on, no matter who does the review, but I think that's a mistake. Nobody is trying to pick on you, they are only trying to help you get your patches in. You should try to see the feedback as something positive.
That's not quite true. I didn't feel like Zeb was picking on me at all: I am genuinely grateful for his feedback, and I told him so. We simply had a disagreement of opinion, and it wasn't even about content, it was just about style. I didn't think it was a big deal until you told me I need to unquestioningly accept whatever feedback I receive.
And if you look at Zeb's commit stats, you will realize that he probably knows a thing or two about getting patches in, and that it would be smart to take his advice.
OK. -Alex
participants (3)
-
Alex Henrie -
Alexandre Julliard -
Zebediah Figura