[Commodity] Commodity that monitors status - code review request

[mateusz_s]

Hi,
Could you please review this commodity code.
I want it to execute some commands in given time interval.

I checked it in the system with cpu idle monitor and it looked fine - cpu was at minimum.

I just want to make sure that the timer and waiting command are done ok,
so the application wont lag the system.

ps.
its not yet finished (and not polished) - but I would like to know if there is some
kind of error in this on this level? the final code will run simple tcp connection
routine to check the internet status and will save it in ENV variable.

Thanks in advance..

#include <proto/exec.h>
#include <proto/dos.h>

#include <clib/alib_protos.h>

#include <clib/commodities_protos.h>
#include <libraries/commodities.h>

// Libs
struct Library* CxBase        = NULL;
struct Library* SocketBase    = NULL;

// Commodity globals
struct NewBroker cx_newbroker = 
{
    NB_VERSION, 					// nb_Version - Version of the NewBroker structure
    (STRPTR)"msInternetStatus",		// nb_Name - Name CX uses to identify this commodity
    (STRPTR)"msInternetStatus v0.1",	// nb_Title - Title of commodity that appears in CXExchange
    (STRPTR)"This Commodity executes any command or script in regular time intervals.",        // nb_Descr - Description of the commodity
    0,          					// nb_Unique - Tells CX not to launch new commodity with same name
    0,          					// nb_Flags - Tells CX if this commodity has a window
    0,          					// nb_Pri - This commodity's priority
    0,          					// nb_Port - MsgPort CX talks to
    0           					// nb_ReservedChannel - reserved for later use
};

struct MsgPort      *cx_broker_message_port;
CxObj               *cx_broker;
ULONG               cx_sign = 0;

// --- timer globals ---
struct MsgPort* timer_message_port;
struct timerequest* timer_io;
struct Message *timer_message;
struct IOExtSer *reply;
ULONG timer_sign = 0;
int numer = 0;

int main(void)
{
	timer_message_port = CreateMsgPort();
	if (timer_message_port == NULL)
		return 0;

	timer_io = (struct timerequest*)CreateExtIO(timer_message_port, sizeof(struct timerequest));
	if (timer_io == NULL)
		return 0;

	if (OpenDevice( TIMERNAME, UNIT_VBLANK, (struct IORequest *)timer_io, 0L ))
		return 0;

    timer_sign = (1L << timer_message_port->mp_SigBit );

     // Before bothering with anything else, open the library
     if ( (CxBase = OpenLibrary((CONST_STRPTR)"commodities.library", 37L)) )
     {
          // Commodities talks to a Commodities application through
          // an Exec Message port, which the application provides
          if ( (cx_broker_message_port = CreateMsgPort()) )
          {
               cx_newbroker.nb_Port = cx_broker_message_port;

               // The commodities.library function CxBroker() adds a
               // broker to the master list.  It takes two arguments,
               // a pointer to a NewBroker structure and a pointer to
               // a LONG.  The NewBroker structure contains information
               // to set up the broker.  If the second argument is not
               // NULL, CxBroker will fill it in with an error code.             
               if ( (cx_broker = CxBroker(&cx_newbroker, NULL)) )
               {
                    // After it's set up correctly, the broker
                    // has to be activated
                    ActivateCxObj(cx_broker, 1L);

                    cx_sign = (1L << cx_broker_message_port->mp_SigBit);

                    timer_io->tr_node.io_Command 	= TR_ADDREQUEST;
                    timer_io->tr_time.tv_secs    	= 0;
                    timer_io->tr_time.tv_micro		= 10;
                    SendIO((struct IORequest *)timer_io);

                    BYTE is_running = 1;

                    // the main processing loop
                    BYTE loop = 1;

                    while(loop)
                    {
                         CxMsg *cx_message;
	                    ULONG cx_message_id;
	                    ULONG cx_message_type;

                         ULONG sign = Wait( timer_sign | cx_sign);

                               if ( (sign & timer_sign) && is_running)
                                {
                                        while( GetMsg(timer_message_port))
                                        {
                                                numer++;
                                                  printf("YES: %d  ", numer);
                                                  timer_io->tr_node.io_Command 	= TR_ADDREQUEST;
                                                  timer_io->tr_time.tv_secs    	= 2;
                                                  timer_io->tr_time.tv_micro		= 0;

                                            SendIO((struct IORequest *)timer_io);
                                        }                                 
                                }

              if (sign & cx_sign)                        
                         while(cx_message = (CxMsg*) GetMsg(cx_broker_message_port))
                         {
                              // Extract necessary information from the CxMessage and return it
                              cx_message_id 	= CxMsgID(cx_message);
                              cx_message_type = CxMsgType(cx_message);

                              ReplyMsg((struct Message *)cx_message);

                              switch(cx_message_type)
                              {
                                   // Commodities has sent a command
                                   case CXM_COMMAND:
                                        switch(cx_message_id)
                                        {
                                             case CXCMD_DISABLE:
                                                  printf("msInternetStatus has been DISABLED.\n");
                                                  ActivateCxObj(cx_broker, 0L);
                                                  is_running = 0;
                                                  break;

                                             case CXCMD_ENABLE:
                                                  printf("msInternetStatus has been ENABLED.\n");
                                                  ActivateCxObj(cx_broker, 1L);
                                                  is_running = 1;
                                                  break;

                                             case CXCMD_KILL:
                                                  printf( "msInternetStatus has been REMOVED from the system.\n");
                                                  loop = 0L;
                                                  is_running = 0;
                                                  break;
                                        }
                                   break;		
                              }
                         }
                    }
                    
                    DeleteCxObj(cx_broker);
               }             
               
               DeletePort(cx_broker_message_port);
          }
          
          CloseLibrary(CxBase);
     }

     //CloseDevice( (struct IORequest*) timer_io);
	//DeleteExtIO( (struct IORequest*) timer_io);
	//DeleteMsgPort(timer_message_port);

	return 0;
}

[tygre]

Hi Mateusz_s!

Thanks for sharing your code!

It looks good to me, but what happens if sign & timer_sign is not 0 and is_running is 0?

Actually, what's the use of is_running in comparison to loop?

Cheers!

[mateusz_s]

Originally Posted by tygre

Hi Mateusz_s!

Thanks for sharing your code!

It looks good to me, but what happens if sign & timer_sign is not 0 and is_running is 0?

Actually, what's the use of is_running in comparison to loop?

Cheers!

A bit corrected and cleaned up version, I think this should be ok.
CPU idle is OK. I can enable/disable the activity.
Probably the timer cleanup is bugged right now because I got crash after exiting.

#include <proto/exec.h>
#include <proto/dos.h>

#include <clib/alib_protos.h>

#include <clib/commodities_protos.h>
#include <libraries/commodities.h>

#include <sys/socket.h>
#include <proto/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <sys/ioctl.h>
#include <stdio.h>

// Application name and version.
#define   APP_NAME            "msIntenetStatus"
#define   APP_VERSION         "v0.1"
#define   APP_DESCRIPTION     "Checks Internet connetion in regular time intervals."     

// Libs
struct Library* CxBase        = NULL;
struct Library* SocketBase    = NULL;

// Commodity globals
struct NewBroker cx_newbroker = 
{
    NB_VERSION, 					// nb_Version - Version of the NewBroker structure
    (STRPTR)APP_NAME,		          // nb_Name - Name CX uses to identify this commodity
    (STRPTR)APP_NAME" "APP_VERSION,	// nb_Title - Title of commodity that appears in CXExchange
    (STRPTR)APP_DESCRIPTION,            // nb_Descr - Description of the commodity
    NBU_UNIQUE,    					// nb_Unique - Tells CX not to launch new commodity with same name
    0,          					// nb_Flags - Tells CX if this commodity has a window
    0,          					// nb_Pri - This commodity's priority
    0,          					// nb_Port - MsgPort CX talks to
    0           					// nb_ReservedChannel - reserved for later use
};

struct MsgPort      *cx_broker_message_port;
CxObj               *cx_broker;
ULONG               cx_signal = 0;


// --- timer globals ---
struct MsgPort* timer_message_port;
struct timerequest* timer_io;
struct Message *timer_message;
struct IOExtSer *reply;
ULONG timer_signal = 0;
int numer = 0;

// Timer functions definitions.
int timer_init()
{
	timer_message_port = CreateMsgPort();
	if (timer_message_port == NULL)
		return 0;

	timer_io = (struct timerequest*)CreateExtIO(timer_message_port, sizeof(struct timerequest));
	if (timer_io == NULL)
		return 0;

	if (OpenDevice(TIMERNAME, UNIT_VBLANK, (struct IORequest *)timer_io, 0L))
		return 0;

     timer_signal = (1L << timer_message_port->mp_SigBit );

     return 1;
}

void timer_send(ULONG _sec, ULONG _micro)
{
     timer_io->tr_node.io_Command 	= TR_ADDREQUEST;

     timer_io->tr_time.tv_micro	= _micro;
     timer_io->tr_time.tv_sec      = 0;     
     timer_io->tr_time.tv_secs    	= _sec;
     timer_io->tr_time.tv_usec     = 0;
     
     SendIO((struct IORequest *)timer_io);
}

void timer_cleanup()
{
	CloseDevice( (struct IORequest*) timer_io);
	DeleteExtIO( (struct IORequest*) timer_io);
	DeleteMsgPort(timer_message_port);
}


int main(void)
{
	// Lets init the timer first.
	if (!timer_init())
	{
		printf("%s: Error! Couldn't create the timer...", APP_NAME);
		timer_cleanup();
		return 1;
	}

     // Before bothering with anything else, open the library.
     if ( (CxBase = OpenLibrary((CONST_STRPTR)"commodities.library", 37L)) )
     {
          // Commodities talks to a Commodities application through
          // an Exec Message port, which the application provides
          if ( (cx_broker_message_port = CreateMsgPort()) )
          {
               cx_newbroker.nb_Port = cx_broker_message_port;

               // The commodities.library function CxBroker() adds a
               // broker to the master list.  It takes two arguments,
               // a pointer to a NewBroker structure and a pointer to
               // a LONG.  The NewBroker structure contains information
               // to set up the broker.  If the second argument is not
               // NULL, CxBroker will fill it in with an error code.             
               if ( (cx_broker = CxBroker(&cx_newbroker, NULL)) )
               {
                    // After it's set up correctly, the broker has to be activated.
                    ActivateCxObj(cx_broker, 1L);

                    cx_signal = (1L << cx_broker_message_port->mp_SigBit);

                    // Send first - short interval to timer.
                    timer_send(0, 1);

                    // Enabled/Disabled status.
                    BYTE cx_enabled = 1;

                    // The main processing loop.
                    BYTE cx_loop = 1;

                    while(cx_loop)
                    {
                         CxMsg *cx_message;
	                    ULONG cx_message_id;
	                    ULONG cx_message_type;

                         // Wait until timer or cx signal appear.
                         Wait( timer_signal | cx_signal);

                         // --------------------------------------------------------------------
                         // --- If we get the signal from the timer and commodity is enabled, 
                         // --- execute our checking routine. 
                         // --------------------------------------------------------------------
                         while(GetMsg(timer_message_port) && cx_enabled)
                         {
                              numer++;
                              printf("YES: %d  ", numer);
                              timer_send(2, 0);
                         }                           
                         // -------------------------------------------------------------

                         // Commodity msg processing                   
                         while(cx_message = (CxMsg*)GetMsg(cx_broker_message_port))
                         {
                              // Extract necessary information from the CxMessage and return it
                              cx_message_id 	= CxMsgID(cx_message);
                              cx_message_type = CxMsgType(cx_message);

                              ReplyMsg((struct Message *)cx_message);

                              switch(cx_message_type)
                              {
                                   // Commodities has sent a command
                                   case CXM_COMMAND:
                                        switch(cx_message_id)
                                        {
                                             case CXCMD_DISABLE:
                                                  ActivateCxObj(cx_broker, 0L);
                                                  cx_enabled = 0;
                                                  break;

                                             case CXCMD_ENABLE:
                                             timer_send(2, 0);
                                                  ActivateCxObj(cx_broker, 1L);
                                                  cx_enabled = 1;
                                                  
                                                  break;

                                             case CXCMD_KILL:
                                                  cx_loop = 0;
                                                  cx_enabled = 0;
                                                  break;
                                        }
                                   break;		
                              }
                         }
                    }
                    
                    DeleteCxObj(cx_broker);
               }             
               
               DeletePort(cx_broker_message_port);
          }
          
          CloseLibrary(CxBase);
     }

	return 0;
}

[a/b]

You're not calling timer_cleanup() before you exit normally (cx kill msg received), plus you should AbortIO() if it's in progress. Also missing a ReplyMsg() for timer msgs.

[mateusz_s]

Originally Posted by a/b

You're not calling timer_cleanup() before you exit normally (cx kill msg received), plus you should AbortIO() if it's in progress. Also missing a ReplyMsg() for timer msgs.

I correct timer quitting using AbortIO() and WaitIO() - not crashing anymore after quitting.

I also saw this thread: https://eab.abime.net/showthread.php?p=1458225
and read info from @Thomas Richter and @thomas
- and I changed GetMsg() into WaitIO()

as for ReplyMsg() I search thru couple of exmapples and no one
used it..

#include <proto/exec.h>
#include <proto/dos.h>

#include <clib/alib_protos.h>

#include <clib/commodities_protos.h>
#include <libraries/commodities.h>

#include <sys/socket.h>
#include <proto/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <sys/ioctl.h>
#include <stdio.h>

// Application name and version.
#define   APP_NAME            "msIntenetStatus"
#define   APP_VERSION         "v0.1"
#define   APP_DESCRIPTION     "Checks Internet connection status."     

// Version that appears in Icon->Information.
static const char  *APP_info_version =	"$VER: Version "APP_VERSION;

// Libs.
struct Library* CxBase        = NULL;
struct Library* SocketBase    = NULL;

// Commodity globals.
struct NewBroker cx_newbroker = 
{
    NB_VERSION, 					// nb_Version - Version of the NewBroker structure
    (STRPTR)APP_NAME,		          // nb_Name - Name CX uses to identify this commodity
    (STRPTR)APP_NAME" "APP_VERSION,	// nb_Title - Title of commodity that appears in CXExchange
    (STRPTR)APP_DESCRIPTION,            // nb_Descr - Description of the commodity
    NBU_UNIQUE,    					// nb_Unique - Tells CX not to launch new commodity with same name
    0,          					// nb_Flags - Tells CX if this commodity has a window
    0,          					// nb_Pri - This commodity's priority
    0,          					// nb_Port - MsgPort CX talks to
    0           					// nb_ReservedChannel - reserved for later use
};

struct MsgPort*     cx_broker_message_port;
CxObj*              cx_broker;
ULONG               cx_signal;

// Timer globals.
struct MsgPort*     timer_message_port;
struct timerequest* timer_io;
ULONG               timer_signal;

int numer = 0;

// Timer functions definitions.
int timer_init()
{
	timer_message_port = CreateMsgPort();
	if (timer_message_port == NULL)
		return 0;

	timer_io = (struct timerequest*)CreateExtIO(timer_message_port, sizeof(struct timerequest));
	if (timer_io == NULL)
		return 0;

	if (OpenDevice(TIMERNAME, UNIT_VBLANK, (struct IORequest *)timer_io, 0L))
		return 0;

     timer_signal = (1L << timer_message_port->mp_SigBit );

     return 1;
}

void timer_send(ULONG _sec, ULONG _micro)
{
     timer_io->tr_node.io_Command 	= TR_ADDREQUEST;

     timer_io->tr_time.tv_micro	= _micro;
     timer_io->tr_time.tv_sec      = 0;     
     timer_io->tr_time.tv_secs    	= _sec;
     timer_io->tr_time.tv_usec     = 0;
     
     SendIO((struct IORequest *)timer_io);
}

void timer_cleanup()
{
     // All I/O requests must be complete before CloseDevice().
     AbortIO(timer_io); 

     // Clean up.
     WaitIO(timer_io);
	CloseDevice( (struct IORequest*) timer_io);     
	DeleteExtIO( (struct IORequest*) timer_io);
	DeleteMsgPort(timer_message_port);
}

// Entry point.
int main(void)
{
	// Lets init the timer first.
	if (!timer_init())
	{
		printf("%s: Error! Couldn't create the timer...", APP_NAME);
		timer_cleanup();
		return 1;
	}

     // Before bothering with anything else, open the library.
     if ( (CxBase = OpenLibrary((CONST_STRPTR)"commodities.library", 37L)) )
     {
          // Commodities talks to a Commodities application through
          // an Exec Message port, which the application provides
          if ( (cx_broker_message_port = CreateMsgPort()) )
          {
               cx_newbroker.nb_Port = cx_broker_message_port;

               // The commodities.library function CxBroker() adds a
               // broker to the master list.  It takes two arguments,
               // a pointer to a NewBroker structure and a pointer to
               // a LONG.  The NewBroker structure contains information
               // to set up the broker.  If the second argument is not
               // NULL, CxBroker will fill it in with an error code.             
               if ( (cx_broker = CxBroker(&cx_newbroker, NULL)) )
               {
                    // After it's set up correctly, the broker has to be activated.
                    ActivateCxObj(cx_broker, 1L);

                    cx_signal = (1L << cx_broker_message_port->mp_SigBit);

                    // Send first - short interval to timer.
                    timer_send(0, 1);

                    // Enabled/Disabled status.
                    BYTE cx_enabled = 1;

                    // The main processing loop.
                    BYTE cx_loop = 1;

                    while(cx_loop)
                    {
                         CxMsg *cx_message;
	                    ULONG cx_message_id;
	                    ULONG cx_message_type;            

                         // Wait until timer or cx signal appear.
			          ULONG signal = Wait(cx_signal | timer_signal);

                         // --------------------------------------------------------------------
                         // --- If we get the signal from the timer and commodity is enabled, 
                         // --- execute our checking routine. 
                         // --------------------------------------------------------------------
                         if ( (signal & timer_signal) && cx_enabled)
                         {
                              // Using WaitIO() to handle request instead of GetMsg(). 
                              WaitIO(timer_io);

                              numer++;
                              printf("YES: %d  ", numer);
                              timer_send(2, 0);
                         }                           
                         // -------------------------------------------------------------

                         // Commodity msg processing  
                         if ( signal & cx_signal )        
                         {         
                              while(cx_message = (CxMsg*)GetMsg(cx_broker_message_port))
                              {
                                   // Extract necessary information from the CxMessage and return it
                                   cx_message_id 	= CxMsgID(cx_message);
                                   cx_message_type = CxMsgType(cx_message);

                                   ReplyMsg((struct Message*)cx_message);

                                   switch(cx_message_type)
                                   {
                                        // Commodities has sent a command
                                        case CXM_COMMAND:
                                             switch(cx_message_id)
                                             {
                                                  case CXCMD_DISABLE:
                                                       AbortIO(timer_io); 
                                                       WaitIO(timer_io);
                                                       ActivateCxObj(cx_broker, 0L);
                                                       cx_enabled = 0;
                                                       break;

                                                  case CXCMD_ENABLE:
                                                       timer_send(2, 0);
                                                       ActivateCxObj(cx_broker, 1L);
                                                       cx_enabled = 1;                                                  
                                                       break;

                                                  case CXCMD_KILL:
                                                       cx_loop = 0;
                                                       cx_enabled = 0;
                                                       break;
                                             }
                                        break;		
                                   }
                              }
                         }
                    }
                    
                    DeleteCxObj(cx_broker);
               }             
               
               DeletePort(cx_broker_message_port);
          }
          
          CloseLibrary(CxBase);
     }

     // Clean up timer.
     timer_cleanup();

	return 0;
}

[a/b]

Originally Posted by mateusz_s

as for ReplyMsg() I search thru couple of exmapples and no one used it..

If' there's no memory leak then it's fine I guess. Otherwise you'll observe increased memory usage with each msg received, so it's easy to test.

[tygre]

Hi Mateusz_s!

Looks good to me! For what it's worth, this is my timer code for AmigaMapPing, which looks very much like your code

Thanks for sharing your code!