cgroup: convert bash to Cpp #401
Conversation
|
Can one of the admins verify this patch? |
api/server/app.cc
Outdated
| LOG_WARNING_("can't create butterfly cgroup, fail cmd '%s'", | ||
| create_dir.c_str()); | ||
| std::string directory = std::string(cgroupPath) + "/butterfly" ; | ||
| if (mkdir((char*)directory.c_str(),S_IWGRP) < 0){ |
There was a problem hiding this comment.
mkdir take a const char * in parameter, so I don't think the cast is necessary.
Also, if you must cast, please use a C++ cast. (const_cast,static_cast,bit_cast,reinterpret_cast)
api/server/app.cc
Outdated
| create_dir.c_str()); | ||
| std::string directory = std::string(cgroupPath) + "/butterfly" ; | ||
| if (mkdir((char*)directory.c_str(),S_IWGRP) < 0){ | ||
| LOG_WARNING_("can't create directory already exists\n"); |
api/server/app.cc
Outdated
| std::ifstream ifs(file1); | ||
| std::ofstream ofs(file2); | ||
| if((ifs.is_open()) && (ofs.is_open())){ | ||
| ofs << ifs.rdbuf(); |
api/server/app.cc
Outdated
| std::ifstream in(fileToRead); | ||
| std::ofstream out(fileToWrite); | ||
| if ((in.is_open()) && (out.is_open())){ | ||
| std::string s; |
api/server/app.cc
Outdated
|
|
||
| int | ||
| main(int argc, char *argv[]) { | ||
| int main(int argc, char *argv[]) { |
There was a problem hiding this comment.
well, that might be right, bit that's unrelated to this commit, so if you want to push that fix, please do it in a separated commit.
outscale-hmi
force-pushed
the
fixCgroup
branch
from
e9f7258 to
35d15fd
Compare
api/server/app.cc
Outdated
| int setCgroups(std::string file1, std::string file2) { | ||
| std::ifstream ifs(file1); | ||
| std::ofstream ofs(file2); | ||
| if ((ifs.is_open()) && (ofs.is_open())) { |
There was a problem hiding this comment.
Could be simplify to:
if (!ifs.is_open() || !ofs.is_open())
return -1;
ofs << ifs.rdbuf();
return 0;
not a big deal, but economise 3 lines of code
api/server/app.cc
Outdated
| std::ofstream out(fileToWrite); | ||
| if ((in.is_open()) && (out.is_open())) { | ||
| std::string s; | ||
| while (getline(in, s)) { |
There was a problem hiding this comment.
this file should contain a number, I don't think the loop is necessary
api/server/app.cc
Outdated
| fileToRead = std::string(cgroupPath) + "/cpuset.mems"; | ||
| if (!access(fileToRead.c_str(), R_OK)) { | ||
| fileToWrite = std::string(cgroupPath) + "/butterfly/cpuset.mems"; | ||
| check = setCgroups(fileToRead, fileToWrite); |
There was a problem hiding this comment.
if (!setCgroups(fileToRead, fileToWrite)) {
LOG_WARNING_("can't set cgroup cpuset.mems");
}
api/server/app.cc
Outdated
| fileToRead = std::string(cgroupPath) + "/cpuset.cpus"; | ||
| fileToWrite = std::string(cgroupPath) + "/butterfly/cpuset.mems"; | ||
| check = setCgroups(fileToRead, fileToWrite); | ||
| if (check != 0) { |
There was a problem hiding this comment.
if (!setCgroups(fileToRead, fileToWrite)) {
LOG_WARNING_("can't set cgroup cpuset.cpus");
}
api/server/app.cc
Outdated
| if (oss.str().compare(word)) { | ||
| LOG_WARNING_("can't properly set cgroup pid"); | ||
| return; | ||
| } else { |
There was a problem hiding this comment.
as you return, you can remove the else
api/server/app.cc
Outdated
| BASH(setStr) { | ||
| int check = 0; | ||
| std::string fileToWrite = std::string(SrcCgroup()) + "/butterfly/tasks"; | ||
| check = setCgroups(oss.str(), fileToWrite); |
There was a problem hiding this comment.
if (!setCgroups(oss.str(), fileToWrite)) {
LOG_WARNING_("can't set cgroup pid");
}
api/server/app.cc
Outdated
| LOG_WARNING_("can't unset task from butterfly cgroup"); | ||
|
|
||
| std::string fileToRead = std::string(SrcCgroup()) + "/butterfly/tasks"; | ||
| std::string fileToWrite = std::string(SrcCgroup()) + "/bin/bash /tasks"; |
There was a problem hiding this comment.
and is wrong here too, sorry, I think it shoud be
std::string(SrcCgroup()) + "/task"
api/server/app.cc
Outdated
| create_dir.c_str()); | ||
| std::string directory = std::string(cgroupPath) + "/butterfly"; | ||
| if (mkdir(directory.c_str(), S_IWGRP) < 0) { | ||
| LOG_WARNING_("can't create directory already exists\n"); |
There was a problem hiding this comment.
I don't think the "already exists" is true,
I mean if mkdir fail, all you know is that you was unable to create the dir, assuming it's because the file alerady exist seems odd to me, you should either use errno or something to know why this fail, or just write "can't create directory: %s", directory.c_str()
| std::string fileToRead = std::string(cgroupPath) + "/cpu.shares"; | ||
| std::string fileToWrite = std::string(cgroupPath) + "/butterfly/cpu.shares"; | ||
| std::ifstream in(fileToRead); | ||
| std::ofstream out(fileToWrite); |
There was a problem hiding this comment.
I think a blank line is needed between declaration and code ?
api/server/app.cc
Outdated
| } | ||
| BASH("rmdir " + std::string(SrcCgroup()) + "/butterfly") { | ||
|
|
||
| int check = 0; |
There was a problem hiding this comment.
well, you can remove this
api/server/app.cc
Outdated
|
|
||
| int check = 0; | ||
| std::string dirToRemove = std::string(SrcCgroup()) + "/butterfly"; | ||
| check = rmdir(dirToRemove.c_str()); |
There was a problem hiding this comment.
if (rmdir(dirToRemove.c_str()) < 0) {
LOG_WARNING_("can't destroy cgroup");
}
also rmdir man said that if rmdir fail, it return -1, so using !check is wrong here, and do the exact opposite of what you want
Signed-off-by: hanen mizouni <[email protected]>
| } | ||
|
|
||
| fileToRead = std::string(cgroupPath) + "/cpuset.cpus"; | ||
| fileToWrite = std::string(cgroupPath) + "/butterfly/cpuset.mems"; |
There was a problem hiding this comment.
s/cpuset.mems/cpuset.cpus/
There was a problem hiding this comment.
this code was useful for centos6, so we might need to test it there before merging
Signed-off-by: hanen mizouni [email protected]