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

add option of saving pid file #217

Merged
merged 7 commits into from
Aug 16, 2015
Merged

add option of saving pid file #217

merged 7 commits into from
Aug 16, 2015

Conversation

hbl0307106015
Copy link

Signed-off-by: Tymon banglang.huang@foxmail.com

I think wifidog-gateway should have a option to save pid file, so I add this function.

Signed-off-by: tymon <tymon@tymon-32bit.(none)>
@hbl0307106015
Copy link
Author

The build is successful in my openwrt-trunk, could you please show me more hints ?

@acv
Copy link
Contributor

acv commented Jun 19, 2015

So there's two things here.

Technically, the code does not fail to build. But the non-SSL integration build are set to convert all warnings to errors. The SSL build don't run with that compiler flag so they succeed.

The error itself is that getpid() returns pid_t which is a signed long on linux. Yet the printf() specifies an unsigned long. %zu might be the right string to use here, I believe that current OpenWrt use a C99 compliant toolchain.

@hbl0307106015
Copy link
Author

thank you for suggestions, I will commit another patch for it later.

@hbl0307106015
Copy link
Author

I 'd already modified the commit just now, please check it.

@mhaas
Copy link
Contributor

mhaas commented Jul 5, 2015

Hey, travis (or rather, the C compiler) does not like some of your code. Can you check that and fix the warning?

Signed-off-by: tymon <banglang.huang@foxmail.com>
@hbl0307106015
Copy link
Author

I can not see the Travis CI build details, could you show me more information so that I can correct my code ?

@hbl0307106015
Copy link
Author

Of course I really want to know where is the warning occur.

@mhaas
Copy link
Contributor

mhaas commented Jul 26, 2015

You can see the build log here: https://travis-ci.org/wifidog/wifidog-gateway/jobs/72659411

Signed-off-by: tymon.huang <banglang.huang@foxmail.com>
@hbl0307106015
Copy link
Author

Ok, I got it, all build error has been fixed.

{
if (pf) {
FILE *f = fopen(pf, "w");
if (f) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide a meaningful error message to the user if the fopen call fails?

@mhaas
Copy link
Contributor

mhaas commented Aug 1, 2015

Much appreciated. I added some comments to your patch - we probably need to check the return values. Otherwise coverity will complain because we're not programming defensively.

…gful message if failed

Signed-off-by: tymon.huang <banglang.huang@foxmail.com>
@hbl0307106015
Copy link
Author

well, thanks mhaas' suggestions, I add some meaningful message to user and check the return value of fclose as well.

}

/* fopen return NULL, open file failed */
debug(LOG_ERR, "fopen() on flie %s was failed (%s)", pf, strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there is a small control flow error here: the call do debug call will always fire regardless of the value of f.

Copy link
Author

Choose a reason for hiding this comment

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

I am sorry to make a stupid mistake.

Signed-off-by: tymon.huang <banglang.huang@foxmail.com>
@hbl0307106015
Copy link
Author

Anything else needed to be modified ?

@mhaas
Copy link
Contributor

mhaas commented Aug 16, 2015

No, it's perfect now. Thanks for the PR and implementing the changes!

mhaas added a commit that referenced this pull request Aug 16, 2015
add option of saving pid file
@mhaas mhaas merged commit ed6aed8 into wifidog:master Aug 16, 2015
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.

4 participants