Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor bug fix for shell scripts like kill_tera.sh, launch_tera.sh and teracli #1280

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lpstudy
Copy link

@lpstudy lpstudy commented Jun 2, 2017

(#1279)

  1. add readline download link to avoid compile error (build.conf.template and build.sh.
  2. run the shell with CURRENT_DIRECTORY, to avoid the error like bin/kill_tera.sh.
  3. add GetProcessDir function to get the exe directory, so that we can run teracli in any directory, rather than only work by ./teracli.

lpstudy added 2 commits June 2, 2017 20:18
1. add readline download link to avoid compile error
2. run the shell with CURRENT_DIRECTORY, to avoid the error like `bin/kill_tera.sh`
3. add GetProcessDir function to get the exe directory, so that we can run `teracli` in any directory, rather than only work by `./teracli`.
}
return buf;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

代码风格:缩进不对,左大括号缺少空格

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

} else{
return dirname(buf);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如上

@@ -1195,7 +1195,7 @@ static int InitFlags(const std::string& confpath, const std::string& log_prefix)
LOG(ERROR) << "should specify no more than one config file";
return -1;
}

std::string exedir = GetProcessDir();
if (!confpath.empty() && IsExist(confpath)){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tera.flag里面会配置logdir,路径不对的话,会导致日志不能输出,可以增加一下判断

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

增加了判断。

} else if (IsExist(utils::GetValueFromEnv("TERA_CONF"))) {
flagfile = utils::GetValueFromEnv("TERA_CONF");
} else {
LOG(ERROR) << "hasn't specify the flagfile, but default config file not found";
return -1;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

代码风格

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

cd -
touch "${FLAG_DIR}/readline_${READLINE_VERSION}"
fi

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果系统已经预装了readline,就不需要下载此tar包重新编译了

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已经增加了判断,我的是centos7.2的系统,但是并没有安装readline库。需要手动安装,其他预装库都正常,我认为其他人也会有相似的情况。

@@ -58,6 +61,7 @@ elif [ $MIRROR == "baidu" ]; then
GPERFTOOLS_URL=http://gitlab.baidu.com/baidups/third/raw/master/gperftools-${GPERFTOOLS_VERSION}.tar.gz
INS_URL=http://gitlab.baidu.com/baidups/third/raw/master/ins-${INS_VERSION}.tar.gz
NOSE_URL=http://gitlab.baidu.com/baidups/third/raw/master/nose-${NOSE_VERSION}.tar.gz
READLINE_URL=http://git.savannah.gnu.org/cgit/readline.git/snapshot/readline-${READLINE_VERSION}.tar.gz
else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

百度内部编译方式不需要使用此包

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已去掉

@@ -12,6 +12,7 @@
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <libgen.h>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google的代码风格

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感谢评论,原来我并没有关注过头文件的包含顺序问题,去调查了一下顺序:OS SDK .h , C标准库、C++标准库、其它库的头文件、自己工程的头文件。于是按照这个顺序重新组织了一下,如下:

#include "common/file/file_path.h"

#include <dirent.h>
#include <grp.h>
#include <libgen.h>
#include <pwd.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include <gflags/gflags.h>

#include "common/base/string_ext.h"

return "";
} else{
return dirname(buf);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dirname返回一个char*,是否需要释放;如果不用释放,是否是线程安全的?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不需要释放,线程安全的。dirname可能返回内部的static alloc的内存,也可能返回buf的内存。buf本身是局部变量,可以确保线程安全,dirname本身是线程安全函数。

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果是内部的静态分配,为什么是线程安全的


std::string GetProcessDir(){
char buf[1024];
ssize_t count = readlink("/proc/self/exe", buf, 1024);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

路径长度最大是1024吗

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已设置为linux/limits.h中的PATH_MAX常量

see the comments for the details.
LOG(ERROR) << "wrong log directory: "<<FLAGS_log_dir;
return -1;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以报warning,不应该直接new client失败;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants