naviserver-devel

Re: [naviserver-devel] naviserver/nsd driver.c,1.119,1.120

[Stephen D. <sd...@gm...>]

On Wed, Oct 22, 2008 at 4:23 AM, Vlad Seryakov
<ser...@us...> wrote:
> Update of /cvsroot/naviserver/naviserver/nsd
> In directory fdv4jf1.ch3.sourceforge.com:/tmp/cvs-serv22913/nsd
>
> Modified Files:
>        driver.c
> Log Message:
> new accept status, SSL driver works now
>
>
> Index: driver.c
> ===================================================================
> RCS file: /cvsroot/naviserver/naviserver/nsd/driver.c,v
> retrieving revision 1.119
> retrieving revision 1.120
> diff -C2 -d -r1.119 -r1.120
> *** driver.c    20 Oct 2008 00:06:54 -0000      1.119
> --- driver.c    22 Oct 2008 03:23:06 -0000      1.120
> ***************
> *** 787,791 ****
>   * Results:
>   *      _ACCEPT:       a socket was accepted, poll for data
> !  *      _ACCEPT_DATA:  a socket was accepted, data present
>   *      _ACCEPT_ERROR: no socket was accepted
>   *
> --- 787,793 ----
>   * Results:
>   *      _ACCEPT:       a socket was accepted, poll for data
> !  *      _ACCEPT_DATA:  a socket was accepted, data present, read immediately
> !  *                     if in async mode, defer reading to connection thread
> !  *      _ACCEPT_QUEUE: a socket was accepted, queue immediately
>   *      _ACCEPT_ERROR: no socket was accepted
>   *
> ***************
> *** 1523,1534 ****
>
>      } else {
>          drvPtr->queuesize++;
>
> -         /*
> -          * If there is already data present then read it without
> -          * polling if we're in async mode.
> -          */
> -
>          if (status == NS_DRIVER_ACCEPT_DATA) {
>              if (drvPtr->opts & NS_DRIVER_ASYNC) {
>                  status = SockRead(sockPtr, 0);
> --- 1525,1538 ----
>
>      } else {
> +         status = SOCK_MORE;
>          drvPtr->queuesize++;
>
>          if (status == NS_DRIVER_ACCEPT_DATA) {
> +
> +             /*
> +              * If there is already data present then read it without
> +              * polling if we're in async mode.
> +              */
> +
>              if (drvPtr->opts & NS_DRIVER_ASYNC) {
>                  status = SockRead(sockPtr, 0);
> ***************
> *** 1541,1553 ****
>
>                  /*
> !                  *  We need to call this to make sure socket has request structure allocated,
> !                  *  otherwise NsGetRequest will call SockRead which is not what this driver wants
>                   */
>
> -                 SockPrepare(sockPtr);
>                  status = SOCK_READY;
>              }
> !         } else {
> !             status = SOCK_MORE;
>          }
>      }
> --- 1545,1564 ----
>
>                  /*
> !                  * Queue this socket without reading, NsGetRequest in
> !                  * the connection thread will perform actual reading of the request
>                   */
>
>                  status = SOCK_READY;
>              }
> !         } else
> !         if (status == NS_DRIVER_ACCEPT_QUEUE) {
> !
> !             /*
> !              *  We need to call SockPrepare to make sure socket has request structure allocated,
> !              *  otherwise NsGetRequest will call SockRead which is not what this driver wants
> !              */
> !
> !             SockPrepare(sockPtr);
> !             status = SOCK_READY;
>          }
>      }


Why the new accept status: NS_DRIVER_ACCEPT_QUEUE ?

The idea we talked about with the driver callbacks was that a
non-standard driver could fake-up a request in it's read callback. So,
whereas at the mo you have something like this:

modules/nsudp/nsudp.c:

static NS_DRIVER_ACCEPT_STATUS
Accept(Ns_Sock *sock, SOCKET listensock, struct sockaddr *sockaddrPtr,
int *socklenPtr)
{
    sock->sock = listensock;
    return NS_DRIVER_ACCEPT_DATA;
}

static ssize_t
Recv(Ns_Sock *sock, struct iovec *bufs, int nbufs, Ns_Time
*timeoutPtr, int flags)
{
     socklen_t size = sizeof(struct sockaddr_in);

     return recvfrom(sock->sock, bufs->iov_base, bufs->iov_len, 0,
(struct sockaddr*)&sock->sa, &size);
}


You would instead do something like this:


static ssize_t
Recv(Ns_Sock *sock, struct iovec *bufs, int nbufs, Ns_Time
*timeoutPtr, int flags)
{
    static const char request[] = "GET /whateva HTTP/1.0\r\n\r\n";
    size_t requestLen = sizeof(request);
    socklen_t sockLen = sizeof(struct sockaddr_in);

    memcpy(bufs->iov_base, request, requestLen);

    return recvfrom(sock->sock,
                    bufs->iov_base + requestLen,
                    bufs->iov_len - requestLen,
                    0, (struct sockaddr*) &sock->sa, &size);
}

(checking for buffer lengths skipped here)


The advantage of doing it this way is that everything is much simpler
from the driver threads point of view. It doesn't need to know
anything special about non-standard protocols, and there isn't
anything in the driver callback api that isn't also useful to the HTTP
server.


Also, this can't work:

nsd/driver.c:1515

status = DriverAccept(sockPtr);

if (status == NS_DRIVER_ACCEPT_ERROR) {
    status = SOCK_ERROR;
    ...

} else {
    status = SOCK_MORE;
    ...

    if (status == NS_DRIVER_ACCEPT_DATA) {
        ...
    } else if (status == NS_DRIVER_ACCEPT_QUEUE) {
        ...
    }
}

Re: [naviserver-devel] naviserver/nsd driver.c,1.119,1.120

[Vlad S. <vl...@cr...>]

> 
> static ssize_t
> Recv(Ns_Sock *sock, struct iovec *bufs, int nbufs, Ns_Time
> *timeoutPtr, int flags)
> {
>     static const char request[] = "GET /whateva HTTP/1.0\r\n\r\n";
>     size_t requestLen = sizeof(request);
>     socklen_t sockLen = sizeof(struct sockaddr_in);
> 
>     memcpy(bufs->iov_base, request, requestLen);
> 
>     return recvfrom(sock->sock,
>                     bufs->iov_base + requestLen,
>                     bufs->iov_len - requestLen,
>                     0, (struct sockaddr*) &sock->sa, &size);
> }
> 
> (checking for buffer lengths skipped here)
> 
> 
> The advantage of doing it this way is that everything is much simpler
> from the driver threads point of view. It doesn't need to know
> anything special about non-standard protocols, and there isn't
> anything in the driver callback api that isn't also useful to the HTTP
> server.


Arguable this is not very clear and simple, driver will have to hack 
buffers and every request should pretend to be HTTP just so driver 
thread would think that it serves only HTTP requests.

If we have special codes and already have 3, does another one breaks 
anythings if only one code makes overall process simpler not only in 
driver thread but for drivers itself?

The only reason of new code because SOCK_READY does queue socket 
immediately but because request structure is not set, NsGetRequest will 
call SockRead/SockParse. Basically ACCEP_QEUEU code is the same as 
ACCEPT_DATA but it skips hacking HTTP request buffer and let the conn 
thread perform processing where ACCEPT_DATA assumes HTTP request and 
assumes HTTP flow to be performed.



> Also, this can't work:
> 
> nsd/driver.c:1515
> 
> status = DriverAccept(sockPtr);
> 
> if (status == NS_DRIVER_ACCEPT_ERROR) {
>     status = SOCK_ERROR;
>     ...
> 
> } else {
>     status = SOCK_MORE;
>     ...
> 
>     if (status == NS_DRIVER_ACCEPT_DATA) {
>         ...
>     } else if (status == NS_DRIVER_ACCEPT_QUEUE) {
>         ...
>     }
> }

Oops, my bad

Re: [naviserver-devel] naviserver/nsd driver.c,1.119,1.120

[Stephen D. <sd...@gm...>]

On Wed, Oct 22, 2008 at 8:36 PM, Vlad Seryakov <vl...@cr...> wrote:
>>
>> static ssize_t
>> Recv(Ns_Sock *sock, struct iovec *bufs, int nbufs, Ns_Time
>> *timeoutPtr, int flags)
>> {
>>     static const char request[] = "GET /whateva HTTP/1.0\r\n\r\n";
>>     size_t requestLen = sizeof(request);
>>     socklen_t sockLen = sizeof(struct sockaddr_in);
>>
>>     memcpy(bufs->iov_base, request, requestLen);
>>
>>     return recvfrom(sock->sock,
>>                     bufs->iov_base + requestLen,
>>                     bufs->iov_len - requestLen,
>>                     0, (struct sockaddr*) &sock->sa, &size);
>> }
>>
>> (checking for buffer lengths skipped here)
>>
>>
>> The advantage of doing it this way is that everything is much simpler
>> from the driver threads point of view. It doesn't need to know
>> anything special about non-standard protocols, and there isn't
>> anything in the driver callback api that isn't also useful to the HTTP
>> server.
>
>
> Arguable this is not very clear and simple, driver will have to hack
> buffers and every request should pretend to be HTTP just so driver
> thread would think that it serves only HTTP requests.


It's clear and simple. It's maybe not very pretty :-)


> If we have special codes and already have 3, does another one breaks
> anythings if only one code makes overall process simpler not only in
> driver thread but for drivers itself?


It's a trade off.

Either non-standard drivers prepend their first read with 20 bytes in Recv().
(~ two lines of code)

*OR*

You need to change all parts of the sever that expect an HTTP request
structure to be available. AND you need to account for the fact that
sometimes it gets created in the normal fashion, but other times it
gets created later, or not at all.


You've tried to ways to do this:


1) Exposing a new Ns_DriverSetRequest routine.

Here's what it looked like in nssyslogd:

http://naviserver.cvs.sourceforge.net/viewvc/naviserver/modules/nssyslogd/nssyslogd.c?revision=1.18&view=markup

NS_EXPORT int
Ns_ModuleInit(char *server, char *module)
{
    ...
    Ns_RegisterRequest(server, "SYSLOG",  "/", SyslogRequestProc,
NULL, srvPtr, 0);
    ...
}

static NS_DRIVER_ACCEPT_STATUS
Accept(Ns_Sock *sock, SOCKET listensock, struct sockaddr *sockaddrPtr,
int *socklenPtr)
{
    sock->sock = listensock;
    Ns_DriverSetRequest(sock, "SYSLOG / HTTP/1.0");
    return NS_DRIVER_ACCEPT_DATA;
}


How is this functionally different to what I proposed (example in Recv above)?


2) Doing away with the need for a Ns_Request all together.

Which now looks like this:

http://naviserver.cvs.sourceforge.net/viewvc/naviserver/modules/nssyslogd/nssyslogd.c?revision=1.19&view=markup

NS_EXPORT int
Ns_ModuleInit(char *server, char *module)
{
    ...
    init.requestProc = Request;
    ...
    init.opts = NS_DRIVER_ASYNC | NS_DRIVER_NOPARSE;
    ...
}

static int
Request(void *arg, Ns_Conn *conn)
{
    Ns_DString *dsPtr = Ns_ConnSockContent(conn);
    SyslogRequest *req = SyslogRequestCreate(server, sockPtr->sock,
dsPtr->string, dsPtr->length, &sa);
    SyslogRequestProcess(req);
    ...
}


The strategy here seems to be to completely ignore the standard
request structure and do your own thing. There's nothing wrong with
this in the abstract -- you're not actually using any of the HTTP
stuff. However, the rest of the server fully expects the HTTP request
to be present and filled in, and this is why you've had to touch
dozens of files in dozens of places fixing up all code that might trip
over a null request field. (And the public API has changed...)


As far as I can see, neither method 1 or 2 achieves anything than
passing "METHOD / HTTP/1.0\r\n\r\n" does, using considerably less
code.

For example, In the snippet above, you can see the newly added
Ns_ConnSockContent which returns a pointer to the read buffer (which
is otherwise private). Ordinarily the buffer would contain the HTTP
request line, any headers, then any body. You can get at the body with
the existing Ns_ConnContent. So if you just return "SYSLOG /
HTTP/1.0\r\n\r\n" and then your bytes in Recv(), the request would be
constructed and then you could parse the protocol beginning in the
body content.

Re: [naviserver-devel] naviserver/nsd driver.c,1.119,1.120

[Vlad S. <vl...@cr...>]

I will try to take a look at this. With new API i could not figure out 
about pre-apending request data in Recv callback, so i had to come up 
with the solution which is almost the same as before.

It is just that now i need in my driver to keep track when actual binary 
data starts and i have to force driver to parse data that nobody need 
anyway, using more memory. Empty request now is supported, the work is 
done once and server can server requests without HTTP request at all.
Is this so bad?

Stephen Deasey wrote:
> On Wed, Oct 22, 2008 at 8:36 PM, Vlad Seryakov <vl...@cr...> wrote:
>>> static ssize_t
>>> Recv(Ns_Sock *sock, struct iovec *bufs, int nbufs, Ns_Time
>>> *timeoutPtr, int flags)
>>> {
>>>     static const char request[] = "GET /whateva HTTP/1.0\r\n\r\n";
>>>     size_t requestLen = sizeof(request);
>>>     socklen_t sockLen = sizeof(struct sockaddr_in);
>>>
>>>     memcpy(bufs->iov_base, request, requestLen);
>>>
>>>     return recvfrom(sock->sock,
>>>                     bufs->iov_base + requestLen,
>>>                     bufs->iov_len - requestLen,
>>>                     0, (struct sockaddr*) &sock->sa, &size);
>>> }
>>>
>>> (checking for buffer lengths skipped here)
>>>
>>>
>>> The advantage of doing it this way is that everything is much simpler
>>> from the driver threads point of view. It doesn't need to know
>>> anything special about non-standard protocols, and there isn't
>>> anything in the driver callback api that isn't also useful to the HTTP
>>> server.
>>
>> Arguable this is not very clear and simple, driver will have to hack
>> buffers and every request should pretend to be HTTP just so driver
>> thread would think that it serves only HTTP requests.
> 
> 
> It's clear and simple. It's maybe not very pretty :-)
> 
> 
>> If we have special codes and already have 3, does another one breaks
>> anythings if only one code makes overall process simpler not only in
>> driver thread but for drivers itself?
> 
> 
> It's a trade off.
> 
> Either non-standard drivers prepend their first read with 20 bytes in Recv().
> (~ two lines of code)
> 
> *OR*
> 
> You need to change all parts of the sever that expect an HTTP request
> structure to be available. AND you need to account for the fact that
> sometimes it gets created in the normal fashion, but other times it
> gets created later, or not at all.
> 
> 
> You've tried to ways to do this:
> 
> 
> 1) Exposing a new Ns_DriverSetRequest routine.
> 
> Here's what it looked like in nssyslogd:
> 
> http://naviserver.cvs.sourceforge.net/viewvc/naviserver/modules/nssyslogd/nssyslogd.c?revision=1.18&view=markup
> 
> NS_EXPORT int
> Ns_ModuleInit(char *server, char *module)
> {
>     ...
>     Ns_RegisterRequest(server, "SYSLOG",  "/", SyslogRequestProc,
> NULL, srvPtr, 0);
>     ...
> }
> 
> static NS_DRIVER_ACCEPT_STATUS
> Accept(Ns_Sock *sock, SOCKET listensock, struct sockaddr *sockaddrPtr,
> int *socklenPtr)
> {
>     sock->sock = listensock;
>     Ns_DriverSetRequest(sock, "SYSLOG / HTTP/1.0");
>     return NS_DRIVER_ACCEPT_DATA;
> }
> 
> 
> How is this functionally different to what I proposed (example in Recv above)?
> 
> 
> 2) Doing away with the need for a Ns_Request all together.
> 
> Which now looks like this:
> 
> http://naviserver.cvs.sourceforge.net/viewvc/naviserver/modules/nssyslogd/nssyslogd.c?revision=1.19&view=markup
> 
> NS_EXPORT int
> Ns_ModuleInit(char *server, char *module)
> {
>     ...
>     init.requestProc = Request;
>     ...
>     init.opts = NS_DRIVER_ASYNC | NS_DRIVER_NOPARSE;
>     ...
> }
> 
> static int
> Request(void *arg, Ns_Conn *conn)
> {
>     Ns_DString *dsPtr = Ns_ConnSockContent(conn);
>     SyslogRequest *req = SyslogRequestCreate(server, sockPtr->sock,
> dsPtr->string, dsPtr->length, &sa);
>     SyslogRequestProcess(req);
>     ...
> }
> 
> 
> The strategy here seems to be to completely ignore the standard
> request structure and do your own thing. There's nothing wrong with
> this in the abstract -- you're not actually using any of the HTTP
> stuff. However, the rest of the server fully expects the HTTP request
> to be present and filled in, and this is why you've had to touch
> dozens of files in dozens of places fixing up all code that might trip
> over a null request field. (And the public API has changed...)
> 
> 
> As far as I can see, neither method 1 or 2 achieves anything than
> passing "METHOD / HTTP/1.0\r\n\r\n" does, using considerably less
> code.
> 
> For example, In the snippet above, you can see the newly added
> Ns_ConnSockContent which returns a pointer to the read buffer (which
> is otherwise private). Ordinarily the buffer would contain the HTTP
> request line, any headers, then any body. You can get at the body with
> the existing Ns_ConnContent. So if you just return "SYSLOG /
> HTTP/1.0\r\n\r\n" and then your bytes in Recv(), the request would be
> constructed and then you could parse the protocol beginning in the
> body content.
> 
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> Build the coolest Linux based applications with Moblin SDK & win great prizes
> Grand prize is a trip for two to an Open Source event anywhere in the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> _______________________________________________
> naviserver-devel mailing list
> nav...@li...
> https://lists.sourceforge.net/lists/listinfo/naviserver-devel
> 

Re: [naviserver-devel] naviserver/nsd driver.c,1.119,1.120

[Stephen D. <sd...@gm...>]

On Wed, Oct 22, 2008 at 10:35 PM, Vlad Seryakov <vl...@cr...> wrote:
>>>> static ssize_t
>>>> Recv(Ns_Sock *sock, struct iovec *bufs, int nbufs, Ns_Time
>>>> *timeoutPtr, int flags)
>>>> {
>>>>     static const char request[] = "GET /whateva HTTP/1.0\r\n\r\n";
>>>>     size_t requestLen = sizeof(request);
>>>>     socklen_t sockLen = sizeof(struct sockaddr_in);
>>>>
>>>>     memcpy(bufs->iov_base, request, requestLen);
>>>>
>>>>     return recvfrom(sock->sock,
>>>>                     bufs->iov_base + requestLen,
>>>>                     bufs->iov_len - requestLen,
>>>>                     0, (struct sockaddr*) &sock->sa, &size);
>>>> }
>>>>
>>>> (checking for buffer lengths skipped here)
>>>>
>>>>
>>>> The advantage of doing it this way is that everything is much simpler
>>>> from the driver threads point of view. It doesn't need to know
>>>> anything special about non-standard protocols, and there isn't
>>>> anything in the driver callback api that isn't also useful to the HTTP
>>>> server.
>>>
>>> Arguable this is not very clear and simple, driver will have to hack
>>> buffers and every request should pretend to be HTTP just so driver
>>> thread would think that it serves only HTTP requests.
>>
>>
>> It's clear and simple. It's maybe not very pretty :-)
>>
>>
>>> If we have special codes and already have 3, does another one breaks
>>> anythings if only one code makes overall process simpler not only in
>>> driver thread but for drivers itself?
>>
>>
>> It's a trade off.
>>
>> Either non-standard drivers prepend their first read with 20 bytes in Recv().
>> (~ two lines of code)
>>
>> *OR*
>>
>> You need to change all parts of the sever that expect an HTTP request
>> structure to be available. AND you need to account for the fact that
>> sometimes it gets created in the normal fashion, but other times it
>> gets created later, or not at all.
>>
>>
>> You've tried to ways to do this:
>>
>>
>> 1) Exposing a new Ns_DriverSetRequest routine.
>>
>> Here's what it looked like in nssyslogd:
>>
>> http://naviserver.cvs.sourceforge.net/viewvc/naviserver/modules/nssyslogd/nssyslogd.c?revision=1.18&view=markup
>>
>> NS_EXPORT int
>> Ns_ModuleInit(char *server, char *module)
>> {
>>     ...
>>     Ns_RegisterRequest(server, "SYSLOG",  "/", SyslogRequestProc,
>> NULL, srvPtr, 0);
>>     ...
>> }
>>
>> static NS_DRIVER_ACCEPT_STATUS
>> Accept(Ns_Sock *sock, SOCKET listensock, struct sockaddr *sockaddrPtr,
>> int *socklenPtr)
>> {
>>     sock->sock = listensock;
>>     Ns_DriverSetRequest(sock, "SYSLOG / HTTP/1.0");
>>     return NS_DRIVER_ACCEPT_DATA;
>> }
>>
>>
>> How is this functionally different to what I proposed (example in Recv above)?
>>
>>
>> 2) Doing away with the need for a Ns_Request all together.
>>
>> Which now looks like this:
>>
>> http://naviserver.cvs.sourceforge.net/viewvc/naviserver/modules/nssyslogd/nssyslogd.c?revision=1.19&view=markup
>>
>> NS_EXPORT int
>> Ns_ModuleInit(char *server, char *module)
>> {
>>     ...
>>     init.requestProc = Request;
>>     ...
>>     init.opts = NS_DRIVER_ASYNC | NS_DRIVER_NOPARSE;
>>     ...
>> }
>>
>> static int
>> Request(void *arg, Ns_Conn *conn)
>> {
>>     Ns_DString *dsPtr = Ns_ConnSockContent(conn);
>>     SyslogRequest *req = SyslogRequestCreate(server, sockPtr->sock,
>> dsPtr->string, dsPtr->length, &sa);
>>     SyslogRequestProcess(req);
>>     ...
>> }
>>
>>
>> The strategy here seems to be to completely ignore the standard
>> request structure and do your own thing. There's nothing wrong with
>> this in the abstract -- you're not actually using any of the HTTP
>> stuff. However, the rest of the server fully expects the HTTP request
>> to be present and filled in, and this is why you've had to touch
>> dozens of files in dozens of places fixing up all code that might trip
>> over a null request field. (And the public API has changed...)
>>
>>
>> As far as I can see, neither method 1 or 2 achieves anything than
>> passing "METHOD / HTTP/1.0\r\n\r\n" does, using considerably less
>> code.
>>
>> For example, In the snippet above, you can see the newly added
>> Ns_ConnSockContent which returns a pointer to the read buffer (which
>> is otherwise private). Ordinarily the buffer would contain the HTTP
>> request line, any headers, then any body. You can get at the body with
>> the existing Ns_ConnContent. So if you just return "SYSLOG /
>> HTTP/1.0\r\n\r\n" and then your bytes in Recv(), the request would be
>> constructed and then you could parse the protocol beginning in the
>> body content.
>>
>
>
> I will try to take a look at this. ...
>


Did you take a look at this?  What do you think?

Re: [naviserver-devel] naviserver/nsd driver.c,1.119,1.120

[Vlad S. <vl...@cr...>]

>>> For example, In the snippet above, you can see the newly added
>>> Ns_ConnSockContent which returns a pointer to the read buffer (which
>>> is otherwise private). Ordinarily the buffer would contain the HTTP
>>> request line, any headers, then any body. You can get at the body with
>>> the existing Ns_ConnContent. So if you just return "SYSLOG /
>>> HTTP/1.0\r\n\r\n" and then your bytes in Recv(), the request would be
>>> constructed and then you could parse the protocol beginning in the
>>> body content.
>>>
>>
>> I will try to take a look at this. ...
>>
> 
> 
> Did you take a look at this?  What do you think?

I did and still did not come to any conclusions. The biggest concern is 
in order to make driver simple all drivers will be complicated and look 
like a hacks except nssock :-))
I want other dirvers to be first class citizens the same way as nssock 
which means driver needs to do a little be more than to be pure simple 
HTTP server.