Thread: Issue reading program arguments

Someone tell if this output makes sense to them in relation to the code below (start with bottom function - "ProcessArgv") it because it sure doesn't to me.

Output:

Code:

make run ARGS="--detailed=2 --quiet-zlib -f ./pngsuite/s01n3p01.png"
...
./a.out --detailed=2 --quiet-zlib -f ./pngsuite/s01n3p01.png
main.c:506: Analysing argument '--quiet-zlib'
main.c:506: Analysing argument '-f'
main.c:421: ArgHasIssue( '--quiet-zlib', '(null)' )
main.c:506: Analysing argument './pngsuite/s01n3p01.png'
main.c:421: ArgHasIssue( '--quiet-zlib', './pngsuite/s01n3p01.png' )
view/png.c:330:  IDAT bytes =    31
...

Code:

#define OUTPUT_ERRORS 1
#define OUTPUT_VERBOSE 2
#define OUTPUT_DETAILED 4
int shut_output = 0;
ALLOC l_alloc = {0};
char *image_file = "./openstreetmap.png";

void SetVerbose( char const *path )
{
	if ( path )
	{
		if ( strcmp( path, "2" ) == 0 )
		{
			l_alloc.verbose = stderr;
			return;
		}

		if ( strlen( path ) > 1 )
		{
			l_alloc.verbose = fopen( path, "w" );
			shut_output |= OUTPUT_VERBOSE;
			return;
		}
	}

	l_alloc.verbose = stdout;
}

int ArgHasIssue( char *arg, char *val )
{
	ECHO
	(
		l_alloc.errors,
		fprintf( l_alloc.errors, "ArgHasIssue( '%s', '%s' )\n", arg, val )
	);

	if ( !val )
	{
		val = strchr( arg, '=' );

		if ( val )
			++val;
	}

	if ( strstr( arg, "--quiet-zlib" ) == arg )
	{
		quiet_zlib = true;
		return 0;
	}

	if ( strstr( arg, "--verbose" ) == arg )
	{
		SetVerbose( val );
		l_alloc.errors = l_alloc.verbose;
		return 0;
	}

	if ( strstr( arg, "--detailed" ) == arg )
	{
		if ( !l_alloc.verbose )
		{
			SetVerbose( val );
			l_alloc.errors = l_alloc.verbose;
		}

		l_alloc.detailed = l_alloc.verbose;
		return 0;
	}

	if ( arg[1] == 'f' )
	{
		if ( !val )
		{
			ECHO
			(
				l_alloc.errors,
				fprintf( l_alloc.errors, "No path declared '%s'\n", val )
			)
			return EINVAL;
		}

		if ( access(val, 0) != 0 )
		{
			ECHO
			(
				l_alloc.errors,
				fprintf( l_alloc.errors, "Couldn't find '%s'\n", val )
			)
			return EACCES;
		}

		image_file = val;
		return 0;
	}

	return EINVAL;
}

int ArgHadIssue( int err, char *arg )
{
	FILE *errors = l_alloc.errors;
	ECHO( errors, ECHO_ERR( errors, err ) );
	ECHO( errors, fprintf( errors, "Invalid argument '%s'\n", arg ) );
	fprintf( errors, "Usage: APP [--verbose,--detailed,-f PATH]\n" );
	return EXIT_FAILURE;
}

int ProcessArgv( int argc, char **argv )
{
	int a;
	char *prv = NULL;

	ECHO( l_alloc.errors, fprintf( l_alloc.errors, "ProcessArgv( %c, %p )\n", argc, (void*)argv ) );

	for ( a = 0; a < argc; ++a )
	{
		int err;
		char *arg = argv[a];

		ECHO( l_alloc.errors, fprintf( l_alloc.errors, "Analysing argument '%s'\n", arg ) );

		if ( !arg )
			continue;

		if ( arg[0] == '-' )
		{
			if ( prv )
			{
				err = ArgHasIssue( prv, NULL );

				if ( err )
					return ArgHadIssue( err, arg );

				continue;
			}

			if ( strchr( arg, '=' ) )
			{
				err = ArgHasIssue( arg, NULL );

				if ( err )
					return ArgHadIssue( err, arg );

				continue;
			}

			prv = arg;
			continue;
		}

		if ( !prv )
		{
			if ( a > 0 )
				return ArgHadIssue( EINVAL, arg );
			continue;
		}

		err = ArgHasIssue( prv, arg );

		if ( err )
			return ArgHadIssue( err, arg );

		prv = NULL;
	}

	if ( prv )
	{
		int err = ArgHasIssue( prv, NULL );

		if ( err )
			return ArgHadIssue( err, prv );
	}

	return 0;
}

And your reason for not using getopt is?

Originally Posted by Salem

And your reason for not using getopt is?

Only seen that name once years ago, forgot all about it until now, the fact that I chose not to use it then indicates to me it couldn't satisfy a requirement of mine, I suspect it was a portability issue like not being part of the C standard, anyways it appears the prints were executing but due to a crash that happens later it somehow got ignored. Either way I've chosen a slightly simpler method of argument handling:

Code:

int ArgHasIssue( int a, int num, char *argv[] )
{
	char *arg = (a >= 0) ? argv[a] : NULL;
	char *val = (arg && num) ? argv[a+num] : NULL;

	ECHO
	(
		l_alloc.errors,
		fprintf
		(
			l_alloc.errors,
			"ArgHasIssue( %d, %d, %p ) arg = '%s', val = '%s'\n",
			a,
			num,
			(void*)argv,
			arg ? arg : argv[0],
			val
		)
	);

	if ( !arg )
		return 0;

	if ( !val )
	{
		val = strchr( arg, '=' );

		if ( val )
			++val;
	}

	if ( strstr( arg, "--q-zlib" ) )
	{
		quiet_zlib = true;
		return 0;
	}

	if ( strstr( arg, "--verbose" ) )
	{
		SetVerbose( val );
		l_alloc.errors = l_alloc.verbose;
		return 0;
	}

	if ( strstr( arg, "--detailed" ) )
	{
		if ( !l_alloc.verbose )
		{
			SetVerbose( val );
			l_alloc.errors = l_alloc.verbose;
		}

		l_alloc.detailed = l_alloc.verbose;
		return 0;
	}

	if ( arg[1] == 'f' )
	{
		if ( !val )
		{
			ECHO
			(
				l_alloc.errors,
				fprintf( l_alloc.errors, "No path declared '%s'\n", val )
			)
			return EINVAL;
		}

		if ( access(val, 0) != 0 )
		{
			ECHO
			(
				l_alloc.errors,
				fprintf( l_alloc.errors, "Couldn't find '%s'\n", val )
			)
			return EACCES;
		}

		image_file = val;
		return 0;
	}

	return EINVAL;
}

int ArgHadIssue( int err, int a, int num, char *argv[] )
{
	int stop = a + num;
	FILE *errors = l_alloc.errors;
	ECHO( errors, ECHO_ERR( errors, err ) );
	ECHO( errors, fprintf( errors, "Invalid argument/s: " ) );

	do fprintf( errors, " %s", argv[a++] ); while ( a < stop );

	fprintf( errors, "\nUsage: APP [--verbose,--detailed,-f PATH]\n" );
	return EXIT_FAILURE;
}

int ProcessArgv( int argc, char **argv )
{
	int a, i = -1, num = 0, err;

	ECHO( l_alloc.errors, fprintf( l_alloc.errors, "ProcessArgv( %d, %p )\n", argc, (void*)argv ) );

	for ( a = 0; a < argc; ++a )
	{
		char *arg = argv[a];

		ECHO( l_alloc.errors, fprintf( l_alloc.errors, "Analysing argument '%s'\n", arg ) );

		if ( !arg || arg[0] == '-' )
		{
			if ( num )
			{
				err = ArgHasIssue( i, num, argv );

				if ( err )
					return ArgHadIssue( err, i, num, argv );
			}

			i = a;
			num = 0;

			if ( arg && strchr( arg, '=' ) )
			{
				err = ArgHasIssue( i, num, argv );

				if ( err )
					return ArgHadIssue( err, i, num, argv );
			}

			continue;
		}

		++num;
	}

	if ( num )
	{
		err = ArgHasIssue( i, num, argv );

		if ( err )
			return ArgHadIssue( err, i, num, argv );
	}

	return 0;
}

Which is giving me this output at the moment:

Code:

make debug ARGS="--detailed=2 --q-zlib -f ./pngsuite/s01n3p01.png"
...
./d.out --detailed=2 --q-zlib -f ./pngsuite/s01n3p01.png
main.c:517: ProcessArgv( 5, 0x7ffd6453da78 )
main.c:523: Analysing argument './d.out'
main.c:523: Analysing argument '--detailed=2'
main.c:433: ArgHasIssue( -1, 1, 0x7ffd6453da78 ) arg = './d.out', val = '(null)'
main.c:433: ArgHasIssue( 1, 0, 0x7ffd6453da78 ) arg = '--detailed=2', val = '(null)'
main.c:523: Analysing argument '--q-zlib'
main.c:523: Analysing argument '-f'
main.c:523: Analysing argument './pngsuite/s01n3p01.png'
main.c:433: ArgHasIssue( 3, 1, 0x7ffd6453da78 ) arg = '-f', val = './pngsuite/s01n3p01.png'
Attempting to open ./openstreetmap.png
view/png.c:330:  IDAT bytes =    31
buff/zlib.c:814: ExpandZlibArchive( 0x7ffd6453b878 )
last = 0, type = 00
buff/zlib.c:467: leng = 60801, nlen = 449
...
view/png.c:484: ScanLines( 0x7ffd6453b7e0 )
d.out: malloc.c:2539: sysmalloc: Assertion `(old_top == initial_top (av) && old_size == 0) || ((unsigned long) (old_size) >= MINSIZE && prev_inuse (old_top) && ((unsigned long) old_end & (pagesize - 1)) == 0)' failed.
make: *** [GNUmakefile:46: debug] Aborted (core dumped)
Compilation failed.

Trying to figure out why "--q-zlib" is being ignored at the moment

Originally Posted by rstanley

awsdert:
In reference to getopt() and getopt_long():

What requirement?

It may not be part of the C Standard, but is so common, that is not an issue. There are version for Windows as well.

I would NOT consider your code, "a slightly simpler method"! Try coding a version using getopt() or getopt_long().

You realise getopt() & getopt_long() probably also have similarly complex code right? Mine just happens to be in full view, whereas the other 2 you have to hunt it down on the net

Never mind, fixed it, now I just need to fix the source of the crash.

Code:

int ProcessArgv( int argc, char **argv )
{
	int a, i = -1, num = 0, err;

	ECHO( l_alloc.errors, fprintf( l_alloc.errors, "ProcessArgv( %d, %p )\n", argc, (void*)argv ) );

	for ( a = 0; a < argc; ++a )
	{
		char *arg = argv[a];

		ECHO( l_alloc.errors, fprintf( l_alloc.errors, "Analysing argument '%s'\n", arg ) );

		if ( !arg || arg[0] == '-' )
		{
			if ( num || i >= 0 )
			{
				err = ArgHasIssue( i, num, argv );

				if ( err )
					return ArgHadIssue( err, i, num, argv );
			}

			i = a;
			num = 0;

			continue;
		}

		++num;
	}

	if ( num || i >= 0 )
	{
		err = ArgHasIssue( i, num, argv );

		if ( err )
			return ArgHadIssue( err, i, num, argv );
	}

	return 0;
}

In case anyone wants to help me find the source of memory corruption that is causing the crash I'll be uploading my current code in a minute, I'll edit this post to contain the link after

Edit: As promised, here's the link:

Files * 148969b2657cb6a5650f6995ade9a0f133f18880 * Lee Shallis / glEngine * GitLab

What am I missing? 175 lines?

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>


const char verbose_opt[] = "--verbose";
const char detailed_opt[] = "--detailed";


int main(int argc, char *argv[]) {
   int i = 1;

   // Default for verbose off and detailed is off
   int verbose = 0, detailed = 0;
   char *path = NULL;

   while(i != argc) {
     if(strncmp(argv[i], verbose_opt, strlen(verbose_opt))==0) {
         verbose = 1;
         if(argv[i][strlen(verbose_opt)] == '=')
           verbose = atoi(argv[i]+strlen(verbose_opt)+1);
         else if(argv[i][strlen(verbose_opt)] != '\0')
           break;
         i++;
     } else if(strncmp(argv[i], detailed_opt, strlen(detailed_opt))==0) {
         detailed = 1;
         if(argv[i][strlen(detailed_opt)] == '=')
           verbose = atoi(argv[i]+strlen(detailed_opt)+1);
         else if(argv[i][strlen(detailed_opt)] != '\0')
           break;
         i++;
     } else if(strcmp(argv[i],"-f")==0 && i != argc-1) {
         if(path)
             break; // Specifed file name twice
         path = argv[i+1];
         i+=2;
     } else {
         break; // Option not expected
     }
   }

   // Check all options have been processed, and we have a path
   if(path == NULL || i != argc) {
     fprintf( stderr, "Usage: %s [--verbose,--detailed,-f PATH]\n", argv[0] );
     return 1;
   }

   printf("Verbose  %i\n",verbose);
   printf("Detailed %i\n",detailed);
   printf("Path     %s\n",path);
   return 0;
}

Originally Posted by hamster_nz

What am I missing? 175 lines?

Code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>


const char verbose_opt[] = "--verbose";
const char detailed_opt[] = "--detailed";


int main(int argc, char *argv[]) {
   int i = 1;

   // Default for verbose off and detailed is off
   int verbose = 0, detailed = 0;
   char *path = NULL;

   while(i != argc) {
     if(strncmp(argv[i], verbose_opt, strlen(verbose_opt))==0) {
         verbose = 1;
         if(argv[i][strlen(verbose_opt)] == '=')
           verbose = atoi(argv[i]+strlen(verbose_opt)+1);
         else if(argv[i][strlen(verbose_opt)] != '\0')
           break;
         i++;
     } else if(strncmp(argv[i], detailed_opt, strlen(detailed_opt))==0) {
         detailed = 1;
         if(argv[i][strlen(detailed_opt)] == '=')
           verbose = atoi(argv[i]+strlen(detailed_opt)+1);
         else if(argv[i][strlen(detailed_opt)] != '\0')
           break;
         i++;
     } else if(strcmp(argv[i],"-f")==0 && i != argc-1) {
         if(path)
             break; // Specifed file name twice
         path = argv[i+1];
         i+=2;
     } else {
         break; // Option not expected
     }
   }

   // Check all options have been processed, and we have a path
   if(path == NULL || i != argc) {
     fprintf( stderr, "Usage: %s [--verbose,--detailed,-f PATH]\n", argv[0] );
     return 1;
   }

   printf("Verbose  %i\n",verbose);
   printf("Detailed %i\n",detailed);
   printf("Path     %s\n",path);
   return 0;
}

Sure that probably works but I like my method more, it's easier to handle common errors and avoid copying memory unnecessarily. Plus it helps keep main() simpler, and with the amount of test code I currently have in main() for the glengine project I really do need a simpler main() function, I plan to move the code into another file later so that all argument handling has it's own dedicated file, eventually I want have main only handle the initialisation & cleanup of objects that are to be passed around, argument handling does not belong directly in main() for that kind of plan.

Let's me look closely at this reply.

Originally Posted by awsdert

Sure that probably works but I like my method more,

I can't help with that. But what I do know is anybody here can take those 52 lines, run it though their C compiler and have working code. That counts for something.

it's easier to handle common errors...

I don't think that is the case.

...and avoid copying memory unnecessarily.

Where is that unnecessary memory copying? And processing command lines is a once-only thing, so efficiency isn't a real concern at all as far as I can tell?

Plus it helps keep main() simpler,

Nothing stops using a good design to hide make this only a handful of lines main():

Code:

#include <stdio.h>
#include "user_opts.h"


int main(int argc, char *argv[]) {
   struct user_opts opts;
   if(!opts_process_args(&opts, argc, argv)) {
      // opts_process_args is responsible for logging/displaying any issue
      return 1;
   }
   printf("Verbose  %i\n",opts.verbose);
   printf("Detailed %i\n",opts.detailed);
   printf("Path     %s\n",opts.path);
   return 0;
}

and with the amount of test code I currently have in main() for the glengine project I really do need a simpler main() function,

"it helps keep main() simple" and "my main() is already too complex" in the same sentence? So your design techniques and ethos isn't really working out for you, but you refuse to adapt.

I plan to move the code into another file later so that all argument handling has it's own dedicated file,

As I said and shown. this can be done with that command line processing code - here's the header file for it:

Code:

#ifndef __USER_OPTS_H
#define __USER_OPTS_H
struct user_opts {
  int verbose;
  int detailed;
  char *path;
};


int opts_process_args(struct user_opts *opts, int argc, char *argv[]);
#endif

How much simpler an interface can you get?

eventually I want have main only handle the initialisation & cleanup of objects that are to be passed around, argument handling does not belong directly in main() for that kind of plan.

You are 100% wrong here. main() is the entry point to your code. An aspect of argument handling most definitely does belong in main(). Even if you very quickly contract it out to a sub-module it is "main" that ultimately deals with the logic to see if the user has provided acceptable inputs (unless your command line handler want to call exit(), I guess).

Even with everything modularized, only three lines in a separate main() dedicated to command line argument handling it is still less then half the code.

Code:

  14 main.c
  60 user_opts.c
  10 user_opts.h
  84 total

Originally Posted by hamster_nz

I can't help with that. But what I do know is anybody here can take those 52 lines, run it though their C compiler and have working code. That counts for something.

they can do so with mine too, ArgHasIssue() is only processing the arguments as soon as possible, opt*() appears to just dump it in a list like getenv() retrieves from

Originally Posted by hamster_nz

I don't think that is the case.

Depends on viewpoint, I like to be able to get detailed information out of an error, that cannot be done when everything is handed of to some libraries function unless the function is specifically designed to take a callback that allows you to go find and print that information, from what I've seen the opt*() family does not provide that.

Originally Posted by hamster_nz

Where is that unnecessary memory copying? And processing command lines is a once-only thing, so efficiency isn't a real concern at all as far as I can tell?

As previously mentioned from what I've seen the opt*() family appears to dump arguments and their values in a list, that can involve allocations, I don't personally see the need for that, better to just use the arguments & values as is

Originally Posted by hamster_nz

Nothing stops using a good design to hide make this only a handful of lines main():

Guessing you meant to say "Nothing stopping a good design..", again that depends on how one wants to process those arguments to begin with.

Originally Posted by hamster_nz

"it helps keep main() simple" and "my main() is already too complex" in the same sentence? So your design techniques and ethos isn't really working out for you, but you refuse to adapt.

I said SIMPLER, meaning the code was originally part of main, I just shifted it into it's own set of functions so I could find other code inside main quicker (of which I also plan to move after I'm done getting basic pngs to deflate correctly

Originally Posted by hamster_nz

As I said and shown. this can be done with that command line processing code - here's the header file for it:

Code:

#ifndef __USER_OPTS_H
#define __USER_OPTS_H
struct user_opts {
  int verbose;
  int detailed;
  char *path;
};


int opts_process_args(struct user_opts *opts, int argc, char *argv[]);
#endif

How much simpler an interface can you get?

That's isolating all options, I would have to move them after, I prefer to put them where they're supposed to go from the outset

Originally Posted by hamster_nz

You are 100% wrong here. main() is the entry point to your code. An aspect of argument handling most definitely does belong in main(). Even if you very quickly contract it out to a sub-module it is "main" that ultimately deals with the logic to see if the user has provided acceptable inputs (unless your command line handler want to call exit(), I guess).

No, I would have to add special error handling each time I need to add a return statment, I'd rather main just initialise needed objects, call ProcessArgs(), then call something like Main( Buffers ), then de-initialise everything before exiting, then Main() can just return an error code or 0, keeps exit procedures simple because most functions can just return a code and leave memory cleanup to main(), I prefer to design for minimal checks when cleaning up, here's an example of what I mean:

Code:

int main( int argc, char *argv[] )
{
  int err;
  ALLOC Alloc = {0};
  BUFFERS Buffers = {0};
  CORE Core = {0};
 
  Core.Bufers = &Buffers;
  Buffers.Alloc = &Alloc;
  Alloc.errors = stderr;
  err = (ProcessArgs( &Core, argc, argv ) || Main( &Core ));

  EmptyBuffers( &Buffers );
  
  if( err )
  {
    ECHO( Alloc.errors, ECHO_ERR( Alloc.errors, err ) );
    return EXIT_FAILURE;
  }
  
  return EXIT_SUCCESS;
}

That's my goal, brutally simple

Originally Posted by awsdert

they can do so with mine too,

NO THEY CAN NOT.

Nobody (not even the compiler) knows what ALLOC, BUFFER and CORE are and what they do. We only guess at what ECHO does, the code is almost opaque to everybody. Don't bother explaining them. I don't care - learning about them won't make my world richer.

And no, you can't expected to pull whole random repos from random places to answer your question on a message board.

Currently you do not really code in C. You code in some weird, self constructed dialect of C that nobody else speaks and if I am being very kind has minimal intrinsic merit over the basic language itself.

You are like a friend who has decided to learn English through the teachings of Yoda in Star Wars. All the words are right, but the actual ideas are lost, confused and muddied.

It's like if I asked you "what's wrong with this C code? why doesn't it compile":

Code:

int foo(int a) DEF
   WHILE(a > 0) DO
      printf("%i\n",a);
      DECREMENT(a);
   LOOP
ENDDEF

And then go on to argue that it is perfectly valid C - it is my better style of C - it is just that you don't have this header file:

Code:

#define IF(x)  if(x)
#define THEN  {
#define ENDIF }
#define WHILE(x) while(x)
#define DO {
#define LOOP }
#define DEF  {
#define ENDDEF }
#define DECREMENT(x) x--

If you are honest with yourself even said as much yourself:

Someone tell if this output makes sense to them in relation to the code below (start with bottom function - "ProcessArgv") it because it sure doesn't to me.

You need to start writing idiomatic standard C, use the standard library, just use malloc() and free(), use assert(), and getopt(), just use the stdint.h types like uint8_t like the rest of us do. Don't keep putting everything in an needless wrappers and abstractions.

Move to the C all your future co-workers will use, to the C all the projects you will contribute to will use, to the C you will have to use if you want people to collaborate with you.

Sorry if this 'tough love' hurts, but it is my honest thoughts and feelings.

> Insisting on using implementation based APIs just because the rest of the world is using it is the equivalent of shooting oneself in the foot
If you're that anal about things, why are you still using an off the shelf micro processor?

Design your own silicon - c'mon man, you're just not trying hard enough.

You have an interesting journey, but only to you.
Nobody else is going to follow you.

Originally Posted by Salem

> Insisting on using implementation based APIs just because the rest of the world is using it is the equivalent of shooting oneself in the foot
If you're that anal about things, why are you still using an off the shelf micro processor?

Design your own silicon - c'mon man, you're just not trying hard enough.

You have an interesting journey, but only to you.
Nobody else is going to follow you.

I'm not smart enough for that, besides the point of C is to compile down to asm that can be wildy different to target processor, the problem with the C standards is that too many lessons were learned too late, I plan to resolve that with a new library that builds upon those lessons, paw will be designed around the minimal headers concept that standard C headers attempted to follow, my sprintfv will be partially future proofed by relying on a list of accepted format suffixes, I can't think of anything to future proof prefixes but a dynamic list of accept suffixes and their callbacks would indeed help with future proofing, being a custom sprinfv I can even add in a parameter for a target BUFFER object instead of target string, this would make it far easier to get a hold of the memory needed by the functions without sacrificing custom memory allocators (which are needed for safe multi-threaded allocations). By designing a new library there is only one implementation to worry about the details of, instead of the many implementations by many compilers, I'm not saying C itself is a problem but the many implementations of it's "standard" APIs. 1 implementation = 1 standard, many implementations = many standards, that's what I'm getting at with the shooting onself in the foot