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

os/filestore: when print log, use __func__ instead of hard code function name #15261

Merged
merged 1 commit into from Jun 22, 2017

Conversation

Projects
None yet
6 participants
@mychoxin
Contributor

mychoxin commented May 24, 2017

when print log, use func instead of hard code function name

Signed-off-by: mychoxin mychoxin@gmail.com

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 24, 2017

@joscollin

joscollin requested changes May 24, 2017 edited

Can you make it the same format in all the places ? The colon is missing after the func in some places.

@joscollin joscollin added the cleanup label May 24, 2017

@tchaikov

anyway, personally, i am expecting changes like this come up as parts of more significant ones.

if (cct->_conf->filestore_odsync_write) {
dout(0) << "mount INFO: O_DSYNC write is enabled" << dendl;
dout(0) << __func__ << " O_DSYNC write is enabled" << dendl;

This comment has been minimized.

@tchaikov

tchaikov May 24, 2017

Contributor

maybe we can keep INFO to be backward compatible, unless we have a good reason not to do so?

@@ -1866,7 +1866,7 @@ void FileStore::init_temp_collections()
int FileStore::umount()
{
dout(5) << "umount " << basedir << dendl;
dout(5) << __func__ << ": " << basedir << dendl;

This comment has been minimized.

@tchaikov

tchaikov May 24, 2017

Contributor

why are you adding a colon here but not adding it elsewhere?

This comment has been minimized.

@mychoxin

mychoxin May 25, 2017

Contributor

some where there is colon, and other where there is no colon, so I should add colon at all place to keep the same.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 24, 2017

@mychoxin also, please note __func__ is not a macro. it is a constant.

@liewegas liewegas changed the title from when print log, use macro __func__ instead of function name to os/filestore: when print log, use macro __func__ instead of function name May 24, 2017

@mychoxin mychoxin changed the title from os/filestore: when print log, use macro __func__ instead of function name to os/filestore: when print log, use __func__ instead of hard code function name May 25, 2017

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 26, 2017

@tchaikov already modified, please take a look.

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 26, 2017

and @joscollin, please take a look too.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented May 26, 2017

While at it...
Perhaps you could even change this into a macro that looks like:

#define __FUNC__  __func__ << "(" << __LINE << ")" 

Which makes inpointing the log even easier, especially during debugging.
Adn if you'd like to rewrite the output, you need to do that only at one location.

But I'm not sure when func and LINE are evaluated. Could be that the preprocessor already takes care of LINE. and you end up the the linenumber of the macro.
Then you have to start parameterizing it.

@joscollin

This comment has been minimized.

Member

joscollin commented May 26, 2017

@wjwithagen I was going to approve this, but suddenly saw your comment. So Could you please do "Request Changes" ?

@wjwithagen wjwithagen self-requested a review May 26, 2017

@wjwithagen

@mychoxin
You might want to take a look at my proposal, and see if that works for you

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 27, 2017

@wjwithagen good idea

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 27, 2017

@wjwithagen
I tested __LINE__, it works fine(the line number is file line number, not offset function).
and my format will be like: "function:handle_remove_dev, line:343",
because it seems that the format like: "handle_remove_dev(343)" does not be clear enough.

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 31, 2017

@joscollin @wjwithagen ping
any other suggestion?

@joscollin

This comment has been minimized.

Member

joscollin commented May 31, 2017

@mychoxin Your PR contains 2 commits, one from another user. You need a rebase.

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented May 31, 2017

@joscollin @wjwithagen rebase done

@@ -1332,7 +1333,7 @@ int FileStore::read_op_seq(uint64_t *seq)
memset(s, 0, sizeof(s));
int ret = safe_read(op_fd, s, sizeof(s) - 1);
if (ret < 0) {
derr << "error reading " << current_op_seq_fn << ": " << cpp_strerror(ret) << dendl;
derr << __FUNC__ << "error reading " << current_op_seq_fn << ": " << cpp_strerror(ret) << dendl;

This comment has been minimized.

@joscollin

joscollin May 31, 2017

Member

Missed colon

FDRef fd;
int r = lfn_open(cid, oid, false, &fd);
if (r < 0) {
dout(10) << "FileStore::read(" << cid << "/" << oid << ") open error: "
dout(10) << __FUNC__ << "(" << cid << "/" << oid << ") open error: "

This comment has been minimized.

@joscollin

joscollin May 31, 2017

Member

Missed colon or is it intentional ?

This comment has been minimized.

@mychoxin

mychoxin Jun 4, 2017

Contributor

missed

@@ -3267,7 +3268,7 @@ int FileStore::_do_fiemap(int fd, uint64_t offset, size_t len,
while (i < fiemap->fm_mapped_extents) {
struct fiemap_extent *next = extent + 1;
dout(10) << "FileStore::fiemap() fm_mapped_extents=" << fiemap->fm_mapped_extents
dout(10) << __FUNC__ << "() fm_mapped_extents=" << fiemap->fm_mapped_extents

This comment has been minimized.

@joscollin

joscollin May 31, 2017

Member

Check the format here.

@@ -107,6 +107,7 @@ using ceph::crypto::SHA1;
#define XATTR_SPILL_OUT_NAME "user.cephos.spill_out"
#define XATTR_NO_SPILL_OUT "0"
#define XATTR_SPILL_OUT "1"
#define __FUNC__ "function:" << __func__ << ", line:" << __LINE__

This comment has been minimized.

@wjwithagen

wjwithagen May 31, 2017

Contributor

@mychoxin
I would not make the FUNC string too long. aan somethings like:
func_so_and_so(12345) would already be informative.
The other thing you might want to check, is whether __func__ actually prints the function name, or that is also has the class prefix. (I think it does not have the prefix)

This comment has been minimized.

@mychoxin

mychoxin Jun 12, 2017

Contributor

@wjwithagen __func__ has no class prefix, and there is no other way to just get className::funName, __PRETTY_FUNCTION__ is too informative, may be i can trim from __PRETTY_FUNCTION__, so what's your opinion?

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented Jun 2, 2017

@wjwithagen @joscollin
__func__ has no class prefix, but __PRETTY_FUNCTION__is something good that we can use, here is my examples:
for a class member function, it prints like:
int head_info_parser::parse_info()
for a global function, it prints like:
int handle_remove_dev(void*, dev_info*)
it includes return type, parameter type lists and class name if there exists.

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented Jun 3, 2017

@wjwithagen for default we use __func__, and if someone wants to debug and needs the complete function name then he can open the comment to use __PRETTY_FUNCTION__

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented Jun 7, 2017

@@ -3193,10 +3195,10 @@ int FileStore::read(
bufferptr bptr(len); // prealloc space for entire read
got = safe_pread(**fd, bptr.c_str(), len, offset);
if (got < 0) {
dout(10) << "FileStore::read(" << cid << "/" << oid << ") pread error: " << cpp_strerror(got) << dendl;
dout(10) << __FUNC__ << "(" << cid << "/" << oid << ") pread error: " << cpp_strerror(got) << dendl;

This comment has been minimized.

@joscollin

joscollin Jun 7, 2017

Member

Missed colon or is it intentional ?

This comment has been minimized.

@mychoxin

mychoxin Jun 8, 2017

Contributor

missed colon, i think the parenthesis do not like parameter list

lfn_close(fd);
if (!(allow_eio || !m_filestore_fail_eio || got != -EIO)) {
derr << "FileStore::read(" << cid << "/" << oid << ") pread error: " << cpp_strerror(got) << dendl;
derr << __FUNC__ << "(" << cid << "/" << oid << ") pread error: " << cpp_strerror(got) << dendl;

This comment has been minimized.

@joscollin

joscollin Jun 7, 2017

Member

Missed colon or is it intentional ?

This comment has been minimized.

@mychoxin

mychoxin Jun 8, 2017

Contributor

missed colon, i think the parenthesis do not like parameter list

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented Jun 8, 2017

@joscollin @wjwithagen please take a look.

@mychoxin

This comment has been minimized.

Contributor

mychoxin commented Jun 9, 2017

@mychoxin mychoxin closed this Jun 17, 2017

@mychoxin mychoxin reopened this Jun 17, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jun 17, 2017

@wjwithagen ping?

@@ -107,6 +107,8 @@ using ceph::crypto::SHA1;
#define XATTR_SPILL_OUT_NAME "user.cephos.spill_out"
#define XATTR_NO_SPILL_OUT "0"
#define XATTR_SPILL_OUT "1"
#define __FUNC__ __func__ << "(" << __LINE__ << ")"
//#define __FUNC__ __PRETTY_FUNCTION__ << "(" << __LINE__ << ")"

This comment has been minimized.

@joscollin

joscollin Jun 18, 2017

Member

Could you please remove the commented code ? This was not there when I approved the changes !!!

This comment has been minimized.

@mychoxin

mychoxin Jun 18, 2017

Contributor

I removed it.

when print log, use __func__ instead of hard code function name
Signed-off-by: mychoxin <mychoxin@gmail.com>
@joscollin

This comment has been minimized.

Member

joscollin commented Jun 19, 2017

Jenkins Retest this please

@liewegas liewegas merged commit 4c8c3e9 into ceph:master Jun 22, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details

@wjwithagen wjwithagen referenced this pull request Jun 23, 2017

Closed

osd: Log audit #15701

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment