Feedback request on wininet patch

[Victor Pelt]

Hey

I implemented one of the missing options in InternetSetOption. Any
feedback about the way i implemented it would be appreciated. If it is
alright i will implement the other missing options too

regards
Victor Pelt

[Mike McCormack]

Victor Pelt wrote:
> -
>    FIXME("Option INTERNET_OPTION_RECEIVE_TIMEOUT: STUB\n");
> -
>  {

It would be nice if you could use indentation consistent with the rest 
of the file, and avoid tabs.
> - unsigned long conns=*(unsigned long*)lpBuffer;

What's 'conns' for?
> - /* have HINTERNET hInternet , handle where to set the options */
> - int result = setsockopt(INET_getSocketHelper(lpwhh), INET_getProtocolHelper(lpwhh), SO_RCVTIMEO, lpBuffer, (int)sizeof(lpBuffer));
> -
> - if (result == 0) {
> -
>  ret = TRUE;
> -
>    } else {
> -
>  /* Translate the result in the right error to return */
> -
>  ret = FALSE;
> -
>    }
> -
>  }

How about "ret = (result == 0);"?
> @@ -2455,6 +2504,9 @@ BOOL WINAPI InternetSetOptionW(HINTERNET
>      case INTERNET_OPTION_CONNECT_RETRIES:
>          FIXME("Option INTERNET_OPTION_CONNECT_RETRIES: STUB\n");
>          break;
> - case INTERNET_OPTION_CONTEXT_VALUE:
> - FIXME("Option INTERNET_OPTION_CONTEXT_VALUE; STUB\n");
> - break;
> default:
>     FIXME("Option %ld STUB\n",dwOption);
>     INTERNET_SetLastError(ERROR_INVALID_PARAMETER);

This chunk should be in a separate patch, since it's unrelated.
> +/* Helper functions to get unix backend stuff based in windows handles (LPWININETHANDLEHEADER s) */
> +int INET_getSocketHelper(LPWININETHANDLEHEADER);
> +int INET_getProtocolHelper(LPWININETHANDLEHEADER);

Why not just make these static, and get rid of these declarations?

Mike

[Vijay Kiran Kamuju]

Hi Victor,

If you are interested in implementing more features.
You can have a look at my wininet pacthes, almost all of them have
been rejected.
I hope you might improve those or get new ideas from those.
As i am going to pick some other well documented portion in windows api.
Leaving the wininet for the time being, i might come back to it
sometime lateer (in distant future).

bye,
Vijay

On 3/6/06, Mike McCormack  wrote:
> Victor Pelt wrote:
> > -
> >    FIXME("Option INTERNET_OPTION_RECEIVE_TIMEOUT: STUB\n");
> > -
> >  {
>
> It would be nice if you could use indentation consistent with the rest
> of the file, and avoid tabs.
> > -
> > unsigned long conns=*(unsigned long*)lpBuffer;
>
> What's 'conns' for?
> > -
> > /* have HINTERNET hInternet , handle where to set the options */
> > -
> > int result = setsockopt(INET_getSocketHelper(lpwhh), INET_getProtocolHelper(lpwhh), SO_RCVTIMEO, lpBuffer, (int)sizeof(lpBuffer));
> > -
> > -
> > if (result == 0) {
> > -
> >   ret = TRUE;
> > -
> >    } else {
> > -
> >   /* Translate the result in the right error to return */
> > -
> >   ret = FALSE;
> > -
> >    }
> > -
> >  }
>
> How about "ret = (result == 0);"?
> > @@ -2455,6 +2504,9 @@ BOOL WINAPI InternetSetOptionW(HINTERNET
> >      case INTERNET_OPTION_CONNECT_RETRIES:
> >          FIXME("Option INTERNET_OPTION_CONNECT_RETRIES: STUB\n");
> >          break;
> > - case INTERNET_OPTION_CONTEXT_VALUE:
> > -
> > FIXME("Option INTERNET_OPTION_CONTEXT_VALUE; STUB\n");
> > -
> > break;
> > default:
> >        FIXME("Option %ld STUB\n",dwOption);
> >        INTERNET_SetLastError(ERROR_INVALID_PARAMETER);
>
> This chunk should be in a separate patch, since it's unrelated.
> > +/* Helper functions to get unix backend stuff based in windows handles (LPWININETHANDLEHEADER s) */
> > +int INET_getSocketHelper(LPWININETHANDLEHEADER);
> > +int INET_getProtocolHelper(LPWININETHANDLEHEADER);
>
> Why not just make these static, and get rid of these declarations?
>
> Mike

[Robert Shearman]

Victor Pelt wrote:
> @@ -2447,7 +2485,18 @@ BOOL WINAPI InternetSetOptionW(HINTERNET
>    TRACE("Option INTERNET_OPTION_DISABLE_PASSPORT_AUTH: harmless stub, since not enabled\n");
>    break;
>     case INTERNET_OPTION_RECEIVE_TIMEOUT:
> -
>    FIXME("Option INTERNET_OPTION_RECEIVE_TIMEOUT: STUB\n");
> -
>  {
> - unsigned long conns=*(unsigned long*)lpBuffer;
> - /* have HINTERNET hInternet , handle where to set the options */
> - int result = setsockopt(INET_getSocketHelper(lpwhh), INET_getProtocolHelper(lpwhh), SO_RCVTIMEO, lpBuffer, (int)sizeof(lpBuffer));
> -
> - if (result == 0) {
> -
>  ret = TRUE;
> -
>    } else {
> -
>  /* Translate the result in the right error to return */
> -
>  ret = FALSE;
> -
>    }
> -
>  }

It would be much better to implement this in netconnection.c. Perhaps 
calling it something like NETCONNECTION_set_timeout and allow both the 
send and receive timeouts to be set with it.