spcl / rFaaS

rFaaS: a high-performance FaaS platform with RDMA acceleration for low-latency invocations.
https://mcopik.github.io/projects/rfaas/
BSD 3-Clause "New" or "Revised" License
49 stars 15 forks source link

Fix a compilation error: invalid conversion from ‘const char*’ to ‘char*’ #23

Closed William-Mou closed 1 year ago

William-Mou commented 1 year ago

Introduction

This commit cast the ip address and port number to char * in the call to rdma_getaddrinfo function. This is to avoid invalid conversion error and to ensure that the correct arguments are passed to it.

Error Message:

Here is my environment information:

When I followed the readme instructions for the compilation, I encounter following error.

william@art1:~/rFaaS$ cmake -DCMAKE_CXX_COMPILER=g++ -DCMAKE_BUILD_TYPE=Release

william@art1:~/rFaaS$ cmake --build .
Scanning dependencies of target spdlog
[  1%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/spdlog.cpp.o
[  3%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/stdout_sinks.cpp.o
[  5%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/color_sinks.cpp.o
[  7%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/file_sinks.cpp.o
[  9%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/async.cpp.o
[ 10%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/cfg.cpp.o
[ 12%] Building CXX object _deps/spdlog-build/CMakeFiles/spdlog.dir/src/fmt.cpp.o
[ 14%] Linking CXX static library libspdlog.a
[ 14%] Built target spdlog
Scanning dependencies of target rdmalib
[ 16%] Building CXX object CMakeFiles/rdmalib.dir/rdmalib/lib/buffer.cpp.o
[ 18%] Building CXX object CMakeFiles/rdmalib.dir/rdmalib/lib/connection.cpp.o
[ 20%] Building CXX object CMakeFiles/rdmalib.dir/rdmalib/lib/functions.cpp.o
[ 21%] Building CXX object CMakeFiles/rdmalib.dir/rdmalib/lib/rdmalib.cpp.o
/home/William/rFaaS/rdmalib/lib/rdmalib.cpp: In constructor ‘rdmalib::Address::Address(const string&, int, bool)’:
/home/William/rFaaS/rdmalib/lib/rdmalib.cpp:37:48: error: invalid conversion from ‘const char*’ to ‘char*’ [-fpermissive]
   37 |     impl::expect_zero(rdma_getaddrinfo(ip.c_str(), std::to_string(port).c_str(), &hints, &addrinfo));
      |                                        ~~~~~~~~^~
      |                                                |
      |                                                const char*
In file included from /home/William/rFaaS/rdmalib/include/rdmalib/connection.hpp:12,
                 from /home/William/rFaaS/rdmalib/lib/rdmalib.cpp:2:
/usr/include/rdma/rdma_cma.h:706:28: note:   initializing argument 1 of ‘int rdma_getaddrinfo(char*, char*, rdma_addrinfo*, rdma_addrinfo**)’
  706 | int rdma_getaddrinfo(char *node, char *service,
      |                      ~~~~~~^~~~
/home/William/rFaaS/rdmalib/lib/rdmalib.cpp:37:78: error: invalid conversion from ‘const char*’ to ‘char*’ [-fpermissive]
   37 |     impl::expect_zero(rdma_getaddrinfo(ip.c_str(), std::to_string(port).c_str(), &hints, &addrinfo));
      |                                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~^~
      |                                                                              |
      |                                                                              const char*
In file included from /home/William/rFaaS/rdmalib/include/rdmalib/connection.hpp:12,
                 from /home/William/rFaaS/rdmalib/lib/rdmalib.cpp:2:
/usr/include/rdma/rdma_cma.h:706:40: note:   initializing argument 2 of ‘int rdma_getaddrinfo(char*, char*, rdma_addrinfo*, rdma_addrinfo**)’
  706 | int rdma_getaddrinfo(char *node, char *service,
      |                                  ~~~~~~^~~~~~~
make[2]: *** [CMakeFiles/rdmalib.dir/build.make:102: CMakeFiles/rdmalib.dir/rdmalib/lib/rdmalib.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:176: CMakeFiles/rdmalib.dir/all] Error 2
make: *** [Makefile:130: all] Error 2
Yuanmxc commented 1 year ago

Here is my environment information:

I did not encounter your error, I checked the rdma_getaddrinfo function of my header file <rdmalib/rdmalib.hpp>, its first parameter is const char *node, not the char* node shown in your error , I think this may be because your rdma version is too low, you can try to upgrade it.

William-Mou commented 1 year ago

@Origin-yy you are right!

Since I am using a Mellanox CX3 NIC, I can only use MLNX_OFED version 4.7 or earlier. (actually, the version is 41mlnx1-OFED.4.7.3.0.6.49417) I have read the git blame record, and the function declaration before version 12 is different from the current version. Perhaps we can add a parameter in the CMakeList to allow users to customize the version.

Hi @mcopik, Is it a good idea? Thank you!

mcopik commented 1 year ago

@William-Mou @Origin-yy I think we need to support both variants - as @William-Mou already noticed, users can be stuck to a specific version of this library. Furthermore, we target HPC clusters and updating dependencies there might take a lot of work. Updating spdlog is simple; updating librdmacm is not :(

Since we are interfacing with C API that could not accept const char* and will not modify the data, this could be the perfect example of using the forgotten const_cast. What do you think?

You can also parameterize the solution depending on the library version - up to you :-)

William-Mou commented 1 year ago

Hi @mcopik, Thanks for your response! As you mentioned, upgrading librdmacm on an HPC cluster is a challenging task.

To make the code work properly on different versions, I proposed three ideas:

  1. Use const_cast, which is the approach implemented in this PR.
  2. Parameterize the solution based on the library version, as you mentioned.
  3. Implement an overload function, such as the one shown here: return const_cast<char *>(static_cast<char *>(this.ip);.

    method 3 is referenced to "Item 3, Use const whenever possible," on p. 24 of Effective C++, 3d ed by Scott Meyers.

For method 2: I attempted to parameterize the solution using rdmacm_VERSION in CMake to determine the librdmacm version. However, I encountered an issue: on server A, the header file is declared as non-const for version 1.3.41.0, while on server B, it is declared as const for version 1.2.28.0 (Shown in the image below). This discrepancy may be due to server A using MLNX_OFED installation, whereas server B is using APT installation.

If we cannot assert the declaration based on the version, method 2 may not be feasible. And, I believe that the third method is unnecessary due to readability issues :D

All things considered, I have reopened this PR, and the current code should work on all versions of librdmacm.

mcopik commented 1 year ago

@William-Mou LGTM, thank you for your help!