strcmp() causes segfault. My implementation == crap?

[LupusNoctu]

I'm having some trouble with a system tool I'm trying to create for my linux system. Basically, my laptop has a button to toggle my touchpad on and off, but when I press it, nothing happens. I've done ALOT of research and come up with only one answer: There is no answer. I think it's a bug in GNOME right now. My solution: Make an easy work-around by binding a key to run a command. I've tried scripting it in shell, but parsing the feedback from the system tool I use ("synclient -l | grep -i touchpadoff") has proven... a pain in the butt. So I decided to make a binary instead. Now, parsing it for the data I need has proven easy. I just need to get it to a bool so I can use it in a switch in main().


the function I need working is:

string sERROR; // Notice that this is a GLOBAL string.
               // That way I can write errors out in main().
bool TouchpadState() {
	/*
	 * Returns the  following:
	 * 0 || false == Touchpad is on
	 * 1 || true == Touchpad is off
	 * anything else == error
	*/
	FILE * fp;
        // run the system tool and pipe it to get the needed info.
	fp = popen("synclient -l | grep -i touchpadoff", "r");
	// if it doesn't work, send an error to main().
        if (fp == NULL) {
		sERROR = "ERROR: popen(): Could not open stream!\nEXITING!";
		return NULL;
	}
	char sStatus[50];
	fgets(sStatus, 50, fp); // use the pipe.
	pclose(fp); // close the pipe.
	bool bStatus;
        // I know the character I need is at sStatus[30] on my system.
        // Use this knowledge to obtain a proper boolean.
	if (strcmp((char*)sStatus[30], "0") == 0) {bStatus = 0;} // Segfault
	if (strcmp((char*)sStatus[30], "1") == 0) {bStatus = 1;} // Segfault
	
        // return the boolean to main() to be used in a switch.
	return bStatus;
}

As is, this does compile and install. But when I try to run it, it gives "Segmentation Fault". I have debugged it and found that it is happening when it comes to the lines I've marked with "// Segfault". The debugger show it's actually happening somewhere within strcmp(), but I'm betting it's my implementation that is to blame. Any help is appreciated.

Also, if Duoas reads this, PLEASE WEIGH IN. I've read quite a few of your posts, and you seem to be knowledgeable and very intent on doing things cleanly, securely, and properly. I'd even love to give you a copy of my whole source code and have you weigh in on that so I can improve my skills and knowledge.

In all cases, please remember that this IS FOR A LINUX SYSTEM. I do intend to distribute this under GPL for others who have the same issue I do. And I will be adding code to make this more robust, rather than blindly guessing where the data I need is.

[Disch]

why are you casting a char to a pointer? It's almost as if you got a compiler error and just casted around it, which would explain the segfault.

if (strcmp((char*)sStatus[30], "0") == 0) {bStatus = 0; } // sStatus[30] is a char, not a char*

If you are trying to compare the whole string to "0", you'd do this:
if(strcmp(sStatus,"0") == 0)

If you are trying to compare only the 31st character, you'd do this:

if(sStatus[30] == '0')

If you are trying to compare the whole string starting with the 31st character, you'd do this:

if(strcmp(&sStatus[30], "0") == 0)

[LupusNoctu]

As Homer J. Simpson would say: D'OH!

I feel like a moron now. I don't know why I was stuck on strcmp()... Perhaps because I got to this point through several evolutions from crap that was decidedly more complex than it had to be. This always happens to me after I try to do shell scripting too. I get used to the shell's convoluted ways of doing things that make NO sense what so ever, then it haunts me when I try to go back to something that actually makes sense, such as C++ or Java.

Anyways, thank you very much. I'm absolutely ecstatic now that my program runs as intended. I'll be laughing about this one for a while. :D

After I clean up all my debugging stuff, make it headless, and add the ability to write log files, I'll post the complete program here. I would still love further review and input of my code. :)

[Disch]

Glad to hear you got it working. =)

[LupusNoctu]

Well, here's the latest version of my program. If anyone else is having the same problem in Linux that prompted me to make this, please feel free to build and use this on your system. Just remember that for it be useful, you need to set a keybind in GNOME and/or KDE to run this when you press it.

Presently, it is very crude. It has basic logging ability for trying to figure out where things went wrong if it doesn't work. I'd still like to find a way to avoid using system() to make the call to synclient to change the state of the touchpad. I also want to add the ability to turn off the logging function when the program is called without args, thus logging only when requested by the user via a flag such as --log (path to log file) when it's called. It's still lacking the ability to intelligently find the location of the '0' or '1' in char sStatus. This is important because it may change from computer to computer for all I know, and randomly guessing it's the 31st character isn't safe, sane, or consensual. LOL.

Whether you use it or not, please review it and tell me what you think. Thank you. :)
Also, I've added questions for ya'll in my comments.

/* -*- Mode: C; indent-tabs-mode: t; c-basic-offset: 4; tab-width: 4 -*- */
/*
 * main.cc
 * Copyright (C) E. Mark Anderson 2010 <[e-mail]>
 * 
 * bTouch is free software: you can redistribute it and/or modify it
 * under the terms of the GNU General Public License as published by the
 * Free Software Foundation, either version 3 of the License, or
 * (at your option) any later version.
 * 
 * bTouch is distributed in the hope that it will be useful, but
 * WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 * See the GNU General Public License for more details.
 * 
 * You should have received a copy of the GNU General Public License along
 * with this program.  If not, see <http://www.gnu.org/licenses/>.
 */


/* I need to make sure I'm not importing anything I'm not using.
 * Else I'll be distributing a bloated program. Anyone mind double
 * checking these for me and telling me which ones, if any are unneeded?
 */
#include <stdio.h>
#include <iostream>
#include <fstream>
#include <stdlib.h>
#include <string.h>
using namespace std;

void WriteLog(char sMessage[50], int iSeverity)
{
        /* iSeverity is an int that represents how severe a message is.
         * 4 is general messages, 3 is important messages, 2 is warnings,
         * 1 is fatal errors, 0 would be the unlikely case that it screws up 
         * royally and causes the kernel to panic -- or worse.
         * Would I get appreciable memory conservation if I make it a short int?
         */
	ofstream ofLog;
	ofLog.open("/home/mark/btouch.log", ios::app);
	if (ofLog.is_open())
	{
		ofLog << iSeverity << ": " << sMessage << endl;
		ofLog.close();
	} else { cout << "Could not open stream" << endl; }
	// Is this proper? or should I open it once at the start of main()
        // then close it once at the end of main?
	
}
bool TouchpadState() {
	/*
	 * Function to query the system for the touchpad's state.
	 * Returns the  following:
	 * 0 || false == Touchpad is on
	 * 1 || true == Touchpad is off
	 * anything else == error
	*/
	FILE * fp;
	// Open a pipe and run the system query.
	fp = popen("synclient -l | grep -i touchpadoff", "r");
	// Don't assume the pipe is open, could cause problems if we do.
	if (fp == NULL) {
		WriteLog("ERROR: popen(): Could not open stream!", 1);
		return NULL;
	}
	
	char sStatus[50];
	fgets(sStatus, 50, fp); // Grab the returned data from the pipe.
	pclose(fp); // And close it.
	bool bStatus;
	// Parse the data for what we need and use it to set a proper 
	// boolean.
	if (sStatus[30] == '0') {bStatus = 0;}
	if (sStatus[30] == '1') {bStatus = 1;}
	// Return the boolean to main() so it can be used
	// to determin the proper actions.
	return bStatus;
}

void TouchpadOn() {
	WriteLog("Touchpad was OFF. Turning ON.", 3);
	// execute system call to enable touchpad
	system("synclient TouchpadOff=0");
	// Double check it worked.
	bool bState = TouchpadState();
	switch (bState) // Should now be == false
	{
		case 0:
			 WriteLog("Successfuly toggled ON.", 4);
			break;
		case 1: 
			WriteLog("ERROR: TOUCHPAD IS STILL OFF!", 2);
			break;
		default:
			WriteLog("ERROR: TOUCHPAD STATE UNKNOWN!", 1);
			break;
	}
}

void TouchpadOff() {
	WriteLog("Touchpad was ON. Turning OFF.", 3);
	// execute system call to disable touchpad
	system("synclient TouchpadOff=1");
	// Double check it worked.
	bool bState = TouchpadState();
	switch (bState) // Should now be == true
	{
		case 1:
			WriteLog("Successfully toggled OFF.", 4);
			break;
		case 0: 
			WriteLog("ERROR: TOUCHPAD IS STILL ON!", 2);
			break;
		default:
			WriteLog("ERROR: TOUCHPAD STATE UNKNOWN!", 1);
			break;
	}
}

int main() // If I modify this to allow arguments, is there a way to then make it global?
{
	WriteLog("NEW EXECUTION", 4);
	bool bState = TouchpadState(); // Get the touchpad's state.

		switch (bState) // Find out which actions need to be taken.
		{
			case 1:
				TouchpadOn(); // If it's off, turn it on.
				break;
			case 0:
				TouchpadOff(); // If it's on, turn it off.
				break;
			default: // If neither are true, log an error.
				WriteLog("ERROR: Unable to determine state", 1);
				break;
		}
	// Any errors that occured should have been handled by us, therefore
	// we should terminate with success.
	WriteLog("Execution complete\n", 4);
	return 0;
}

[Disch]

	bool bState = TouchpadState(); // Get the touchpad's state.

		switch (bState) // Find out which actions need to be taken.
		{
			case 1:
				TouchpadOn(); // If it's off, turn it on.
				break;
			case 0:
				TouchpadOff(); // If it's on, turn it off.
				break;
			default: // If neither are true, log an error.
				WriteLog("ERROR: Unable to determine state", 1);
				break;
		}

Of course since bState is a bool, the only possible values are true or false (1 or 0), therefore the default case here can never be run.

Which brings to mind another point:

	bool bStatus;
	// Parse the data for what we need and use it to set a proper 
	// boolean.
	if (sStatus[30] == '0') {bStatus = 0;}
	if (sStatus[30] == '1') {bStatus = 1;}

If sStatus[30] is neither '0' nor '1', bStatus will be left uninitialized, resulting it in randomly/unreliably turning the touchpad on or off.

[LupusNoctu]

Of course since bState is a bool, the only possible values are true or false (1 or 0), therefore the default case here can never be run.

So, it will never be NULL even if I only declare it? Then I need to find another way to handle that error if it ever arises. Perhaps change bStatus to iStatus (an int, or perhaps a short int.) and initialize it to a value of 2. Then when we read sStatus to get iStatus, it's set to 0 or 1 according to the return from the system. In this way, if both of my if statements fail, it is left alone and a 2 is returned, allowing the default case to run. That sounds good to me. I'll make the change. :)

I also remembered first thing this morning that globals == EVIL! lol. So ignore the question on line 121. I'll write a class to handle all the log functions. This way I can use Log::bUse = 0 || 1 to directly turn on or off logging in a much safer way and I won't have to pass an extra argument every time I call the logger. This will also make it easier to write more comprehensive logs when asked for by the user.

Today, I might also get around to making TouchpadState() more robust and able to find where the 0 or 1 is on its own, rather than trusting me. lol I might even re-write the functions into a class so that I can use something more along the lines of Touchpad.state() Touchpad.on(), etc. I'll post the new version for review when I'm done for the day. But I think I'll start a new thread for it.

Thanks again for the input, Disch. :)

[Disch]

C++ doesn't have a special "NULL" state like some languages do. "NULL" is basically another way to say 0. So basically... NULL == false.

Also in C++, variables aren't initialized to any state unless you specifically initialize them.

bool foo = false;  // initialized to false

bool bar;  // not initialized

if(bar)  // no way to know whether or not this will run!  bar could be true or false! 

That sounds good to me. I'll make the change. :)

I'd recommend a typed enum instead of relying on magic numbers. magic numbers will work, but they're very easy to forget. enums mean more typing, but they're much more clear.

Might not be a big deal for such a small project, but if you look back at this a few months from now... if(state == 2) isn't as clear as if(state == state_unknown)

[LupusNoctu]

Thank you for clearing that up. And the enums are an even better idea than mine due to the increased human readability. Do I still need the return type of TouchpadState() to be int? I would guess so since, according to what I've been reading, enums are essentially aliases for ints; Allowing you to type state_unknown where you would have typed 2.

Also, I have a question about a matrix I'm wanting to use as a message queue in a class. I'm working on re-writing all the Touchpad functions into a class. As the functions of that class are executed, they build up a message queue for the log in char cMsgs[][] and count the number of messages in int iMsgs. This is easy. Something like this:

int iMsgs = 0;
char cMsgs[][];
Touchpad::state() // I'm trunc'ing this to show just the message parts.
{
      cMsgs[iMsgs++] = "message to log";
      // If I understand correctly, iMsgs++ will allow iMsgs to be read first, then 
      // it will be incremented.
}

main() // Again, trunc'd to just show the message handling.
{
      if (iMsgs > 0)
      {
            for (; iMsgs > 0;) // No init, no progressor. iMsgs already init'd.
            {
                 Log::write(cMsgs[iMsgs--]); // and this is were we're progressed.
                 // Again, I'm using  the decrement after the var so it is read before
                 // being decremented... If I understand correctly.
            }
      }
}

The trouble is, I also want to clear out the messages from cMsgs[][] as this happens. I don't want to unallocate the memory, just clear the data out and reset the indicies. What is the best way of doing this? I could remove the progressor from the call to Log::write(), then after the message is written to the log, I could do cMsgs[iMsgs--] = '\0'. But would that delete everything in the second dimension of the matrix (cMsgs[][this one])? Is this the way? Or is there a better way? Or should I just leave it alone, only resetting iMsgs to 0, and let the new messages overwrite the old? In this case, what happens if the new message is shorter than the old one?

[Disch]

Do I still need the return type of TouchpadState() to be int? I would guess so since, according to what I've been reading, enums are essentially aliases for ints;

It depends if the enum is typed or not. Example:

enum  // an untyped enum
{
  foo, // enum is untyped, so these are ints
  bar
};

enum myenum  // a typed enum
{
  foobar,  // this enum is typed, so these are not ints
  bar  // instead, they're of type 'myenum'
};

Typed enums can be explicitly cast to ints and vice versa, but cannot be implicitly cast, so it's better to use the type instead of int when dealing with typed enums.

Typed enums also add a bit of readability/clarity (an 'int' could be anything, but you know a 'myenum' could only be one of the enum values)

char cMsgs[][];

I hope you realize this won't work ;P. I don't know if you were truncating that code to keep it simple or not.

Anyway, I try to avoid char arrays like the plague. They are such high maintanance and are really easy to screw up and harder to use. The STL provides a decent string class that greatly simplifies things.

As for a message queue, I would turn to STL again and use list. Specifially a list<string>:

#include <list>
#include <string>


std::list< std::string >   myqueue;

//=================

void AddMessage(const std::string& str)
{
  myqueue.push_back( str );
}

void PrintMessages()
{
  while(!myqueue.empty())
  {
    Log::write( myqueue.front().c_str() );
    myqueue.pop_front();
  }
}

I don't want to unallocate the memory, just clear the data out and reset the indicies

Technically my approach above would unallocate memory, but KISS and don't sweat the small stuff. An implementation that tries to prevent memory allocation will be much more complicated and/or prone to errors/buffer overflows.

[LupusNoctu]

Ok, That definitely looks better than what I had in mind. Much less coding to do. lol. I LOVE this site. Between the excellent reference material here and your help, Disch, I actually feel like I'm beginning to truly learn how to program. I don't feel like just another code monkey anymore. :)

Couple questions, what would be significance in saying iVar& as opposed to &iVar? I know putting the reference operator (&) before the object means "memory address of object". Does this change when you put it behind the object being referenced? I ask because I saw you say std::string& in your code and I've not seen it like that before and http://cplusplus.com/doc/tutorial/pointers/ doesn't make mention of it.

Also, for my enums, does the enum declaration need to be made global in order to make a function's return type be of my enum type? Or can I put it in in that function? If it's in the function, do I have to redeclare it in main() so it can be used as a valid type for the variable that the functions return is stored in? Or can I still make that variable a standard int? I do want to change the switch statement in main to be similar to this.:

int main()
{
   Touchpad tp;
   status_t tpState = tp.state(); // where status_t is the name of
                                                  // the typed enum in Touchpad::state()
   switch (tpState)
   {
      case state_on:
        // turn it off
        break;
      case state_off:
        // turn it on
        break;
      case state_unknown:
        // handle the errors and close cleanly.
        break;
    }
}

Of course, It would probably be better to start the switch like switch (tp.state()). Would that require anything different?

[LupusNoctu]

Nevermind about the enum part. I got that figured out through trial and error. I almost have a successful compile too. I'm left with one error and 3 warnings.

main.cc:33: warning: deprecated conversion from string constant to ‘char*’
main.cc:48: warning: deprecated conversion from string constant to ‘char*’
main.cc:53: warning: deprecated conversion from string constant to ‘char*’
touchpad.cc:97: error: request for member ‘write’ in ‘lf’, which is of non-class type ‘Log*’

If you'd rather I send you the actual files, just let me know, but for now I'll try to just show the areas where the problem likely resides.

main.cc

#include <stdio.h> // line 24
#include <stdlib.h>
#include <string.h>
#include "log.h"
#include "touchpad.h"
using namespace std;

int main()
{
	Log lf ("/home/mark/btouch.log"); // line 33
	Touchpad tp;
	enum status_t {state_on, state_off, state_unknown};
	switch (tp.state()) // Find out which actions need to be taken.
	{
		case state_off:
			tp.on(); // If it's off, turn it on.
			tp.send_msgs(&lf);
			break;
		case state_on:
			tp.send_msgs(&lf);
			tp.off(); // If it's on, turn it off.
			break;
		default: // If neither are true, log an error.
			tp.send_msgs(&lf);
			lf.write("ERROR: Unable to determine state"); // line 48
			break;
	}
	// Any errors that occured should have been handled by us, therefore
	// we should terminate with success.
	lf.write("Execution complete. Closing log file.\n"); // line 53
	lf.close();
	return 0;
}

in touchpad.cc

void Touchpad::send_msgs (Log * lf) // line 93
{
	while (!msgQ.empty())
	{
		*lf.write(msgQ.front().c_str()); // line 97
		msgQ.pop_front();
	}
}

in log.cc

void Log::write(char cMsg[])
{
	// Write the log only if it's open.
	if (bOpen = true)
	{
		ofLog << cMsg << endl;
	}
}

touchpad.h and touchpad.cc do #import "log.h"

[Disch]

Couple questions, what would be significance in saying iVar& as opposed to &iVar?

When defining a variable name, the & signifies that it's a reference.

When used with an existing variable, the & gets the address of the variable.

It's a little confusing:

int foo;
int* ptr = &foo;  // ptr points to foo

int& ref = foo;  // ref is a reference to foo

*ptr = 4;  // this assigns 4 to foo, since ptr points to foo
ref = 10;  // this assigns 10 to foo, since ref refers to foo 

In function parameters, "passing by reference" (having references as parameters instead of normal variables) avoids object copies. For basic types like int, float, bool, etc, you're better off passing "by value" (the normal way). For complex types like string, vector, etc where copying the object might be expensive, passing by reference avoids the copy.

Also, for my enums, does the enum declaration need to be made global in order to make a function's return type be of my enum type?

It doesn't necessarily have to be global, but it does have to be of a broader scope than the function body.

What you could do is make it part of your Touchpad class.

main.cc:33: warning: deprecated conversion from string constant to ‘char*’

This is a const correctness thing.

string literals ("This is a string literal because it's in quotes";) are of type const char* not of type char*.

to correct this:

void Log::write(const char cMsg[]) // <- pass by const ptr

Since 'write' doesn't need to change cMsg, it can take it as a const.

touchpad.cc:97: error: request for member ‘write’ in ‘lf’, which is of non-class type ‘Log*

*lf.write(msgQ.front().c_str()); // line 97

'*' has a lower precedent than '.' Example:

// this
*foo.bar();

// is the same as this:
*(foo.bar());  // it tries to call the function first, then dereferences whatever the function returns)
// in your case this is an error because foo is a pointer, and you can't use . with pointers

// to correct this, you want this:
foo->bar();  // the -> operator dereferences pointers

// another, arguably uglier way to do it is like so:
(*foo).bar();  // same as foo->bar() but not as simple looking 

[LupusNoctu]

Thank you very much. I now have a compiled program. It was acting weird at first. It wasn't writing half the log when it was turning off the touchpad, but worked perfectly when it was turning it on. Through use of the debugger, I noticed it was because I was calling stuff in the wrong order in main(). That's fixed now and this program is running flawlessly. I guess I'll add an extra call to Touchpad::send_msgs() to double check that the message queue is infact empty before it terminates.

Now I just need to add the ability for Touchpad::state() to intelligently find where the 0 or 1 is in sStatus. I should also rename that variable to cStatus now that it's a char[]. Oh, can't forget to implement the option to use command line arguments to enable logging only when it's wanted, and select where the log goes. Then I'll be ready to release it. :)

Disch, you have been a GREAT help in all this. I'd like to add you to my AUTHORS file and where ever else credit is given. Is this ok with you, or would you rather I didn't?

[Disch]

Disch, you have been a GREAT help in all this. I'd like to add you to my AUTHORS file and where ever else credit is given. Is this ok with you, or would you rather I didn't?

I'm happy to help.

I have no preference one way or the other with regard to credits. IMO I don't really think it's necessary, but if you really want to I won't stop you. ;P

[LupusNoctu]

Well, after some finishing touches, testing and debugging, I have a releasable program! It can be found here: http://ubuntuforums.org/showthread.php?p=10036150#post10036150

I released it as a linux source package under the GNU General Public License. If you're not familiar with linux but still want to see the source code, no problem. Just grab an archive tool that can read .tar.gz files such as izArc, and open it. You'll find my full source code in the 'src' directory. :)