Shared memory class

Question

I wrote a class that opens a shared memory communication to use in my project, and would like a review.

mylib_com.hpp

#ifndef __MYLIB_COM_HPP
#define __MYLIB_COM_HPP
#include <sys/mman.h> /* For mmap */
#include <fcntl.h> /* For O_* in shm_open */
#include <unistd.h> /* For ftruncate and close*/
#include <string.h> /* For strcpy*/
#include <string>
namespace Myns{
 class Channel{
 public:
 Channel(std::string name, size_t size = sizeof(char)*50);
 ~Channel();
 void write(const char * msg);
 std::string read();
 private:
 int memFd;
 int res;
 u_char * buffer;
 }; 
}
#endif

mylib_com.cpp

#include "mylib_com.hpp"
#include <errno.h>
#include <system_error>
Myns::Channel::Channel(std::string name, size_t size){
 memFd = shm_open(name.c_str(), O_CREAT | O_RDWR, S_IRWXU);
 if (memFd == -1)
 {
 perror("Can't open file");
 throw std::system_error(errno, std::generic_category(), "Couldn't open shared memory");
 }
 res = ftruncate(memFd, size);
 if (res == -1)
 {
 perror("Can't truncate file");
 close(memFd);
 throw std::system_error(errno, std::generic_category(), "Couldn't truncate shared memory");
 }
 buffer = (u_char *) mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, memFd, 0);
 if (buffer == NULL)
 {
 perror("Can't mmap");
 close(memFd);
 throw std::system_error(errno, std::generic_category(), "Couldn't mmap shared memory");
 }
}
Myns::Channel::~Channel(){
 close(memFd);
}
void Myns::Channel::write(const char * msg){
 strcpy((char*)buffer,msg);
}
```

• Toby Speight: Do you have a good reason not to use boost::interprocess?
• uli: Did you forget the read() memberfunction?
• Giuliano: Well, I didnt know I could use boost. Actually never used... For my use i just didnt need read() right now.

Answer 1

• __MYLIB_COM_HPP is a reserved identifier, you're not allowed to use these in your code.
• Instead of including both <string> and <string.h>, you could also use std::strcpy(). Also, you don't need that in the header, move it to the implementation file.
• sizeof (char) is by definition one, because sizeof gives you the size in multiples of that of a char. That part is basically redundant.
• Outputting an error with perror() and throwing it as an exception is kind-of redundant as well. If you don't log an error when you catch it, you're a fool or have reason to ignore it, but you can't make that decision at the place where the error occurs.
• You are using C-style casts. Don't, use the C++ casts (static_cast in the cases here) instead.
• I'm not sure what u_char is, that may not be portable.
• Avoid casting altogether: mmap() returns void*, which you cast to u_char* and store in buffer. In the only case where you use buffer, you cast it to char* first. Don't, just keep it a void* and only cast when you need.
• You have no error checking when writing. You just take the char const* and feed it to strcpy(), without any bounds check.

Answer 2

Just a couple of additional points...

I've not used c++17, but when performing tests against constants, consider putting the constants on the left, rather than the right. This prevents typos from having unexpected consequences. if (res = -1) changes the value of res, whereas if(-1 = res) doesn't compile.

You're keeping variables in your class that seem like they aren't needed. If they're only needed for the constructor, then use local variables. Maybe you have other plans for it, but res looks particularly suspect, since it's always going to be 0 if the class has been constructed and in the code you've supplied it's never read again.