0

I have write the following code , but the code is still fiven me EEERROR message , which tells that the mremap failed to extend the memory.

int main()
{
int size_of_mem = 1024
int fd = shm_open("/myregion", O_CREAT | O_RDWR, S_IRWXO | S_IRUSR | S_IWUSR);
    if (fd == -1)
        printf("ERROR in shm_open \n") ;

    if (ftruncate(fd, size_of_mem) == -1)
        printf("ERROR in ftruncate \n") ;

    int shm_address = mmap(0 , size_of_mem , PROT_READ | PROT_WRITE | PROT_EXEC ,MAP_SHARED , fd , 0) ;

    if (shm_address == MAP_FAILED)
    {
        printf("Error mmapping the file \n");
        exit(EXIT_FAILURE);
    }
   int temp = mremap(shm_address , size_of_mem ,4000 , MREMAP_MAYMOVE) ;
   if( temp < 0)
    {
        printf("EEEEEEEERROR\n") ;
    }
return 0 ; 
}
hbak
  • 1,333
  • 3
  • 10
  • 22

1 Answers1

4

There are a couple of things wrong here.

First, mmap() and mremap() return a void* pointer, which you must not just cast to an int.

Second, the mremap() man page states:

RETURN VALUE

On success mremap() returns a pointer to the new virtual memory area. On error, the value MAP_FAILED (that is, (void *) -1) is returned, and errno is set appropriately.

So your check temp < 0 is wrong. It should be temp == (void*)-1. It is entirely possible that mremap() returns a valid pointer on success that is smaller than zero when cast to an int.

Third, both mmap() and mremap() set the errno (man page) variable when an error occurs. You can read that to get more information about what exactly went wrong. To just output a text error message use the perror() function (man page). Note that you have to #include <errno.h> for that.

Fourth, if you detect an error condition, you always print a message, but you mostly allow execution to continue. That doesn't make sense. If shm_open() failed, you want to return immediately (or call exit(EXIT_FAILURE)). None of the following functions will work if you couldn't even open the SHM file.

Thus, my cleaned up version looks like this:

#include <error.h>

int main()
{
    int size_of_mem = 1024;
    int fd = shm_open("/myregion", O_CREAT | O_RDWR,
                      S_IRWXO | S_IRUSR | S_IWUSR);
    if (fd == -1)
    {
        perror("Error in shm_open");
        return EXIT_FAILURE;
    }

    if (ftruncate(fd, size_of_mem) == -1)
    {
        perror("Error in ftruncate");
        return EXIT_FAILURE;
    }

    void *shm_address = mmap(0, size_of_mem,
                             PROT_READ | PROT_WRITE | PROT_EXEC,
                             MAP_SHARED, fd, 0);
    if (shm_address == MAP_FAILED)
    {
        perror("Error mmapping the file");
        return EXIT_FAILURE;
    }

    void *temp = mremap(shm_address, size_of_mem, 4000, MREMAP_MAYMOVE);
    if(temp == (void*)-1)
    {
        perror("Error on mremap()");
        return EXIT_FAILURE;
    }
    return 0;
}

Note:

  • Correct data types (void*), correct error checking for mremap(), usage of perror() to print more informative error messages, error paths ending execution of the function.
  • Correct/consistent indentation.
  • No spaces before , in function calls.
Christian Aichinger
  • 6,989
  • 4
  • 40
  • 60
  • i applied what you said and it's work ! , but in first point , i have cast the return value to `int` , and it work , also , i does not cast the return value , and it work ! , secondly , HOW memory address could be in negative (even if it cast to int) .. thanks in advanace. – hbak Apr 20 '14 at 09:05
  • 2
    Just because it works does not mean it is correct. It will fail on 64 bit systems, for example. Even on 32 bit architectures, it is still wrong. Pointers go from 0 to 0xFFFFFFFF. If you map that to an ``int``, 0 - 0x7FFFFFFF are positive numbers, 0x80000000-0xFFFFFFFF are negative. That is why both, mapping to an ``int`` is wrong and why the ``temp < 0`` check is wrong, too. It treats valid pointers such as 0x81230000 as errors because they map to negative integers. – Christian Aichinger Apr 20 '14 at 09:11
  • one more question , if i make `temp` be `unsigned int` , the condition 'temp < 0' will work ? – hbak Apr 20 '14 at 09:18
  • No, it will not work, because ``unsigned int`` is always positive, so it can never be ``< 0``. If you insist on casting to an integer type, cast to ``long int`` and still compare with ``-1`` (that will at least work on 64 bit architectures too). But really, ``void*`` or ``char*`` is the right thing to do. Why are you so fixated on avoiding it? – Christian Aichinger Apr 20 '14 at 09:22
  • 1
    it's just about extending my knowledge , and make things clear :) – hbak Apr 20 '14 at 10:06
  • For more information about the pointer <-> integer conversion, refer to [this question](https://stackoverflow.com/questions/7146813/when-is-an-integer-pointer-cast-actually-correct). The issues mentioned in the question text itself are spot-on. – Christian Aichinger Apr 20 '14 at 11:10