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

Adding implementation for runlevel probe on SUSE #369

Merged
merged 1 commit into from
Apr 5, 2016

Conversation

GautamSatish
Copy link
Contributor

This check-in implements the run-level probe for SUSE as was discussed on the mailing list.

If have implemented the function separate from get_runlevel_sysv because the file paths to init.d and the rc directories are different on SUSE and there was no GCC macros to distinguish between RHEL and SUSE.

Also included the linux/limits.h header file in oval_session.c to resolve PATH_MAX.

@openscap-jenkins
Copy link

Can one of the admins verify this patch?

@isimluk
Copy link
Member

isimluk commented Mar 31, 2016

Test this please

(message for our jenkins) 🛀

@jan-cerny
Copy link
Member

@GautamSatish Thanks for your contribution.

I don't like that most of the code is a copy from get_runlevel_sysv(). The two functions differs only a little. We should avoid repeating code. If there is a bug in the code, we will have to fix it in 2 places (which we will definitely forget to do). :-) Could you re-factor this in a way that you don't copy the code, please? Maybe a good idea would be to move the common code to a new static function.

@GautamSatish
Copy link
Contributor Author

@jan-cerny , forgive me, you had already given me this comment on the mailing list, it slipped my mind.

I can add a wrapper to the get_runlevel_sysv() function for each platform and have them initialize the init_path and rc_path variables and then pass them over to it along with a flag to determine if platform is suse. So that this check will be more of a run-time logic.

Or, I could add a new define option in CFLAGS say -D_SUSE=0 and the value needs to be set to 1 when building for SUSE so that we can simply add #if defined(_SUSE_) in the current get_runlevel_sysv() module. I don't see any existing gcc pre-processor macros that can distinguish between Redhat and SUSE.

What do you think?

@jan-cerny
Copy link
Member

@GautamSatish First option sounds better to me.

@GautamSatish
Copy link
Contributor Author

@jan-cerny , I have made the modifications in the second commit. Could you please take a look?

@jan-cerny
Copy link
Member

@GautamSatish Thanks! Now the code looks good to me, Please squash the two commits, do push -f and then I think it can be merged,

@mpreisler mpreisler added this to the 1.2.9 milestone Apr 4, 2016
Updated the runlevel probe with some refactoring.
@GautamSatish
Copy link
Contributor Author

@jan-cerny I think I have merged the way you wanted.

@jan-cerny
Copy link
Member

@GautamSatish Thank you very much. We highly appreciate all efforts that make OpenSCAP usable on other platforms. There are still some minor issues in your code -- an unused variable in get_runlevel_redhat, and some trailing whitespace. We also try to follow the Linux Kernel Coding Style. However, I accept the patch and merge it upstream. Please keep those remarks in mind. We are looking forward to your further contribution.

@jan-cerny jan-cerny merged commit 18bad00 into OpenSCAP:maint-1.2 Apr 5, 2016
@GautamSatish GautamSatish deleted the add_suse_runlevel branch October 6, 2016 04:06
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.

None yet

5 participants