Experimental Physics and
Industrial Control System

Andrew Johnson via Core-talk <[email protected]> · Wed, 18 Sep 2019 16:52:51 -0000

Can the necessary testing be done in modules/libcom/test/epicsErrlogTest.c or something similar? That file already has tests that implement a local log-server, needed to check iocLogPrefix().

Dirk, did you check this against a VxWorks 6.x version using the newer IP stack? We aren't supporting VxWorks 5.5 in EPICS 7 any more, but we do want it to work with the newer OS versions.

Some comments against the diff as well, but note that I haven't reviewed the functionality, just the build and APIs.

Diff comments:

> diff --git a/modules/libcom/src/log/iocLog.c b/modules/libcom/src/log/iocLog.c
> index e62da20..8cb1349 100644
> --- a/modules/libcom/src/log/iocLog.c
> +++ b/modules/libcom/src/log/iocLog.c
> @@ -75,6 +77,14 @@ void epicsShareAPI epicsShareAPI iocLogFlush (void)
>  }
>  
>  /*
> + * logClientDestroy()
> + */
> +static void iocLogClientDestroy (logClientId id)

Your routine here is correctly named iocLogClientDestroy() but the comment above it says logClientDestroy(), missing the initial "ioc". There is another routine in logClient.c called logClientDestroy()...

> +{
> +    errlogRemoveListeners (logClientSendMessage, id);
> +}
> +
> +/*
>   *  iocLogClientInit()
>   */
>  static logClientId iocLogClientInit (void)
> @@ -89,6 +99,10 @@ static logClientId iocLogClientInit (void)
>          return NULL;
>      }
>      id = logClientCreate (addr, port);
> +    if (id != NULL) {
> +        errlogAddListener (logClientSendMessage, id);

Since you've moved the add and remove listeners calls into this file (iocLog.c) should the logClientSendMessage() routine also be moved into here too? The comment in logClient.h implies that it has already been, but that appears to be a lie. The routine could then be made static and maybe even removed from the logClient.h header.

> +        epicsAtExit (iocLogClientDestroy, id);
> +    }
>      return id;
>  }
>  
> diff --git a/modules/libcom/src/log/logClient.c b/modules/libcom/src/log/logClient.c
> index 99ee671..6671d27 100644
> --- a/modules/libcom/src/log/logClient.c
> +++ b/modules/libcom/src/log/logClient.c
> @@ -21,11 +21,9 @@
>  #include <string.h>
>  #include <stdio.h>
>  
> -#define epicsExportSharedSymbols

Your removal of this #define epicsExportSharedSymbols will cause Windows builds to fail. The locClient.c code gets compiled into the same DLL as all of the headers that follow it, so that macro must be defined before the other libCom headers get included. You could add the #include "epicsExport.h" at this point in the file instead, but I wouldn't do that myself. If you do though, please add a comment on the same line that says "defines epicsExportSharedSymbols".

>  #include "dbDefs.h"
>  #include "epicsEvent.h"
>  #include "iocLog.h"
> -#include "errlog.h"
>  #include "epicsMutex.h"
>  #include "epicsThread.h"
>  #include "epicsTime.h"
> @@ -176,60 +175,59 @@ static void sendMessageChunk(logClient * pClient, const char * message) {
>          unsigned msgBufBytesLeft = 
>              sizeof ( pClient->msgBuf ) - pClient->nextMsgIndex;
>  
> -        if ( strSize > msgBufBytesLeft ) {
> -            int status;
> -
> -            if ( ! pClient->connected ) {
> -                break;
> -            }
> -
> -            if ( msgBufBytesLeft > 0u ) {
> -                memcpy ( & pClient->msgBuf[pClient->nextMsgIndex],
> -                    message, msgBufBytesLeft );
> -                pClient->nextMsgIndex += msgBufBytesLeft;
> -                strSize -= msgBufBytesLeft;
> -                message += msgBufBytesLeft;
> -            }
> -
> -            status = send ( pClient->sock, pClient->msgBuf, 
> -                pClient->nextMsgIndex, 0 );
> -            if ( status > 0 ) {
> -                unsigned nSent = (unsigned) status;
> -                if ( nSent < pClient->nextMsgIndex ) {
> -                    unsigned newNextMsgIndex = pClient->nextMsgIndex - nSent;
> -                    memmove ( pClient->msgBuf, & pClient->msgBuf[nSent], 
> -                        newNextMsgIndex );
> -                    pClient->nextMsgIndex = newNextMsgIndex;
> -                }
> -                else {
> -                    pClient->nextMsgIndex = 0u;
> -                }
> -            }
> -            else {
> -                if ( ! pClient->shutdown ) {
> -                    char sockErrBuf[64];
> -                    if ( status ) {
> -                        epicsSocketConvertErrnoToString ( sockErrBuf, sizeof ( sockErrBuf ) );
> -                    }
> -                    else {
> -                        strcpy ( sockErrBuf, "server initiated disconnect" );
> -                    }
> -                    fprintf ( stderr, "log client: lost contact with log server at \"%s\" because \"%s\"\n", 
> -                        pClient->name, sockErrBuf );
> -                }
> -                logClientClose ( pClient );
> -                break;
> -            }
> +        if ( msgBufBytesLeft < strSize && pClient->nextMsgIndex != 0u && pClient->connected)
> +        {
> +            /* buffer is full, thus flush it */
> +            logClientFlush ( pClient );
> +            msgBufBytesLeft = sizeof ( pClient->msgBuf ) - pClient->nextMsgIndex;
>          }
> -        else {
> -            memcpy ( & pClient->msgBuf[pClient->nextMsgIndex],
> -                message, strSize );
> -            pClient->nextMsgIndex += strSize;
> +        if ( msgBufBytesLeft == 0u ) {
> +            fprintf ( stderr, "log client: messages to \"%s\" are lost\n",
> +                pClient->name );
>              break;
>          }
> +        if ( msgBufBytesLeft > strSize) msgBufBytesLeft = strSize;
> +        memcpy ( & pClient->msgBuf[pClient->nextMsgIndex],
> +            message, msgBufBytesLeft );
> +        pClient->nextMsgIndex += msgBufBytesLeft;
> +        strSize -= msgBufBytesLeft;
> +        message += msgBufBytesLeft;
>      }
>  }
>  
> +/*
> + * epicsSockCountUnsentBytes ()
> + * Should go to osd socket support

I agree that this routine should be moved into the various osdSock.c implementations and tests added to the modules/libcom/test/osiSockTest.c program to check that it works. We should also provide a build-time indicator if we find a platform that can't implement it, to allow the test code to skip the tests on that architecture without failing but still fail on architectures that should provide it.

To match our other APIs the routine name should start with "epicsSocket". "CountUnsentBytes" seems a bit long, how about just "epicsSocketUnsentData()", and should it return a size_t instead of int?

> + */
> +#if defined (_WIN32) && WINVER >= _WIN32_WINNT_WIN10
> +#include <mstcpip.h>
> +#endif
> +
> +static int epicsSockCountUnsentBytes(SOCKET sock) {
> +#if defined (_WIN32) && WINVER >= _WIN32_WINNT_WIN10
> +/* Windows 10 Version 1703 / Server 2016 */
> +/* https://docs.microsoft.com/en-us/windows/win32/api/mstcpip/ns-mstcpip-tcp_info_v0 */
> +    DWORD infoVersion = 0, bytesReturned;
> +    TCP_INFO_v0 tcpInfo;
> +    int status;
> +    if ((status = WSAIoctl(sock, SIO_TCP_INFO, &infoVersion, sizeof(infoVersion),
> +        &tcpInfo, sizeof(tcpInfo), &bytesReturned, NULL, NULL)) == 0)
> +        return tcpInfo.BytesInFlight;
> +#elif defined (SO_NWRITE)
> +/* macOS / iOS */
> +/* https://www.unix.com/man-page/osx/2/setsockopt/ */
> +    int unsent;
> +    if (getsockopt(sock, SOL_SOCKET, SO_NWRITE, &unsent) == 0)
> +        return unsent;
> +#elif defined (TIOCOUTQ)
> +/* Linux */
> +/* https://linux.die.net/man/7/tcp */
> +    int unsent;
> +    if (ioctl(sock, TIOCOUTQ, &unsent) == 0)
> +        return unsent;
> +#endif
> +    return 0;
> +}
>  
>  /* 
>   * logClientSend ()

-- 
https://code.launchpad.net/~dirk.zimoch/epics-base/+git/epics-base/+merge/372925
Your team EPICS Core Developers is subscribed to branch epics-base:7.0.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System