Hi David, thanks for continuing to work on this.
What's wrong with the new version?
The first thing to make it acceptable is to write a regression test to go along with it. I think the test should check that gethostbyname(your_host_name) returns something besides 127.0.0.1, _if_ you in fact have a public IP address. Perhaps use SIOCGIFCONF in the test to check whether you do have a public IP.
+ BOOL getpublicIP(char* target,int size)
The name should be spelled get_public_ip or so, and this should be static.
+ if (skfd < 0) + { + WARN("A socket could not be created"); + return FALSE; + }
On any failure you need to SetLastError with an appropriate error.
+ if(ip[0]==127&&ip[1]==0&&ip[2]==0&&ip[3]==0)continue; /*Exclude 127.0.0.1*/ + + sprintf(buffer,"%i.%i.%i.%i", (int)ip[0],(int)ip[1],(int)ip[2],(int)ip[3]);
I think this is specific to little-endian machines, this should succeed on big-endian machines too. (Not all uses of Wine are to run x86 executables.)
+ if(strcmp(ifr[i].ifr_name,"lo")==0)continue; + if(size>strlen(buffer))strcpy(target,buffer);
Some spaces somewhere would be nice :)
+ char myname[1000]; + gethostname(myname,(size_t)1000);
In WS_gethostbyname you pay a penalty of doing a gethostname call every time. I think this could be optimized. For instance, you could only check it if the returned address is 127.0.0.1. Also, I'm not sure about declaring a buffer of 1000 bytes on the stack. It's also unnecessary: according to rfc1035, each name label is at most 255 chars. I assume, at any rate, that gethostname will not return a dotted name, but a single label.
To get this off the stack altogether, you could have a static buffer that's set every time WS_gethostname is called. WS_gethostbyname could use this value instead of calling gethostname(2) itself. I'm not sure whether this optimization is worth it. If you use a static buffer, it needs to be protected by a critical section.
--Juan
__________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
@Juan Since you're concerned about the performance, which is valid, I've also removed all the string-conversions. Important question: Is 1.0.0.127 a valid public IP-Adress? If yes, I'd have to furtherly change the code, adding some htonl, ntohl, or similar.
greetings, David
Since you're concerned about the performance, which is valid, I've also removed all the string-conversions. Important question: Is 1.0.0.127 a valid public IP-Adress?
Yes.
If yes, I'd have to furtherly change the code, adding some htonl, ntohl, or similar.
Yes, and that would allow you to do only one comparison.
- BOOL get_public_ip(DWORD* target)
This still needs to be static. (static BOOL get_public_ip...)
for (i = 0; i < ifc.ifc_len / sizeof(struct ifreq); i++)
Now I remember why I was concerned about this. This isn't portable. In some systems, struct if_req occupies fixed space in ifc, but in others, the size depends on the amount of data in each struct if_req. You need to use ifreq_len. See e.g.: http://source.winehq.org/source/dlls/iphlpapi/ifenum.c#L201
Or, as I suggested before, you could call iphlpapi.GetAdaptersInfo: it already excludes local interfaces, and winsock already depends on iphlpapi.
--Juan
__________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
-
David Nolden -
Juan Lang