[rfc][icedtea-web] read properties values from C part - library edition :)
Adam Domurad
adomurad at redhat.com
Mon Mar 18 12:09:08 PDT 2013
Thanks for sticking with this :-)
On 03/18/2013 01:24 PM, Jiri Vanek wrote:
> [..snip..]
>>>>
>>>>> #include <string.h>
>>>>
>>>> In C++, you should use #include <cstring>, #include <cstdio>,
>>>> #include <cstdlib> -- pretty much
>>>> no '.h' for the standard libra
>
> ugh. I'm afraid I need more hints what to replace with what and what
> will be impact to the code:(
> ry.
No problem. GCC will accept both, but you should use 'c' to prefix
standard C library headers in C++. It's part of the C++ standard. (I
don't think we should do something non-compliant just because the
compiler lets us)
>>>>
>>>>>
>>>>> //public api
>>>>> char* getUserPropertiesFile();
>>>>> int findSystemConfigFile(char* dest);
>>>>> int findCustomJre(char* dest);
>>>>> int getDeployProperty(char* property, char* dest);
>>>>> //end of public api
>>>>>
>>>>> static void popChar(char *c, int x){
>>>>> int i;
>>>>> for ( i = x; i <= strlen(c); i++){//moving also \0
>>>>
>>>> 'strlen' is a relatively expensive operation and should not be used
>>>> in a loop condition.
>>>> This is more of a 'delete' than a pop.
>>>>
>>>> I strongly recommend using std::string, which defines this, eg:
>
> done, but teh benefit is lesser then expected :((
Using char* by itself is very bug-prone; I see plenty benefit.
>
>>>>>
>>>>>
>>>>> char* getUserPropertiesFile(){
>>>>> struct passwd *pw = getpwuid(getuid());
>>>>> wordexp_t exp_result;
>>>>> wordexp("~/.icedtea/deployment.properties", &exp_result, 0);
>>>>> return exp_result.we_wordv[0];
>>>>
>>>> I would like to avoid using these GNU specific functions if possible.
>>>> This has the potential to leak memory as wordfree is never called.
>>>> We can instead use the standard function getenv("HOME") like so:
>
> nope, home can not be defined. It is not an rule. i have used standard
> function reading from psswd.
This is a *POSIX function*, not a *standard C/C++ function* :-).
Anyway, I am fine with this because it truly is architecture specific.
>>>>
>>>> std::string user_properties_file = std::string(getenv("HOME")) +
>>>> /.icedtea/deployment.properties";
>>>> Note for '+' to work properly on C++ strings, one side of the
>>>> expression must be an std::string.
>>>> (Similar to Java -- except unfortunately string constants are not
>>>> std::string's in C++)
>>>>
>>>> You can also check if the returned environment variable is empty as
>>>> follows:
>>>>
>>>> std::string homedir = getenv("HOME");
>>>> if (homedir.empty()) {
>>>> ... fallback code ... (concatenate HOMEDRIVE and HOMEPATH
>>>> environment variables if you want to
>>>> pretend we support Windows :-)
>>>> }
>>>> std::string user_properties_file = homedir +
>>>> "/.icedtea/deployment.properties"
>>>>
>>>>> }
>>>>>
>>>>>
>>>>> static char* getMainPropertiesFile(){
>>>>> return "/etc/.java/deployment/deployment.properties";
>>>>
>>>> const char* should always be used for strings that should not be
>>>> modified. This is in fact not
>>>> valid C++ as-is.
>
> as SString have come to this place, I hope it is better now :(
yes
>>>>
>>>> [..snip..]
>>>> Bad Jiri! This is 2013, don't use fixed size arrays! :-)
>>>> std::string will make your life easier.
>
> have not, but is done :)
:)
>>>>
>>>>> int customJre = findCustomJre(jDest);
>>>>> if (customJre == 0){
>>>>> strncat(jDest,"/lib/deployment.properties",50);
>>>>
>>>> It is good that you're using strncat, but you're just guessing a size.
>>>> std::string will make your life easier.
>>>>
>>>>> if(access(jDest, F_OK ) != -1 ) {
>>>>> strcpy(dest, jDest);
>>>>> return 0; //file found
>>>>> }
>>>>> } else {
>>>>> if(access(getDefaultJavaPropertiesFile(), F_OK ) != -1 ) {
>>>>
>>>> 'access' is (unfortunately) not a standard function. More idiomatic
>>>> is to try and open the file
>>>> for reading and see if we get NULL back, or in the case of an
>>>> fstream, if the fstream is empty.
>
> hmm.. I let it in, I like the function. .What is th ereason for not
> using it?
It is not a standard C/C++ function, it's a POSIX function. While we
don't support non-POSIX platforms as a target officially, please just
check if FILE* is null or if the fstream is empty when opened.
>>>> Note that const char* is frequently passed as a parameter in C++
>>>> because it can be both an
>>>> std::string (via c_str()) or a string literal.
>>>> Note that fstream needs no close call at all, it is cleaned up once
>>>> it goes out of scope -- this
>>>> is IMHO one of the strongest features of C++ (its resource
>>>> management is handled exactly like its
>>>> memory management).
>>>>
>
> Is it still valid?
>
> Ok, here we go with c++, string version....
>
> /me terrified
Terrified of what ? A friendly patch review from an intern ? :-)
> #include <unistd.h>
> #include <sys/types.h>
> #include <pwd.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string>
> #include <algorithm>
> #include <functional>
> #include <cctype>
> #include <locale>
> #include <iostream>
>
> using namespace std;
>
> //public api
> string user_properties_file();
> bool find_system_config_file(string& dest);
> bool find_custom_jre(string& dest);
> bool read_deploy_property_value(string property, string& dest);
> //end of public api
>
>
> // trim from start
> static inline string <rim(string &s) {
> s.erase(s.begin(), find_if(s.begin(), s.end(),
> not1(ptr_fun<int, int>(isspace))));
> return s;
> }
> // trim from end
> static inline string &rtrim(string &s) {
> s.erase(find_if(s.rbegin(), s.rend(), not1(ptr_fun<int,
> int>(isspace))).base(), s.end());
> return s;
> }
> // trim from both ends
> static inline string &trim(string &s) {
> return ltrim(rtrim(s));
> }
Don't use inline here, it's not necessary or useful.
Well these are funny, they look pasted from stack overflow :-). You only
use trim(), here's something more readable from Stackoverflow that
doesn't need <algorithm>:
|std::string trim(const std::string& str) {
size_t start = str.find_first_not_of(|||" \t"),||||end = str.find_last_not_of(|||" \t");|
|
if (start == std::string::npos) {
return "";
}
return str.substr(start, end - start + 1);
}
|
Why are you returning the reference? In-effect the function signature is a lie :) Either mutate it or assign a copy (like my version does)
|[nit] 'string& s' preferably over 'string &s'. 'string&' is read as the
type, not part of the variable name.
|
>
>
>
> static void remove_all_spaces(string& str)
> {
> for(int i=0; i<str.length(); i++)
> if(str[i] == ' ' || str[i] == '\n' ) str.erase(i,1);
> }
Some braces /indenting would be nice.
>
> static bool get_property_value(string c, string& dest){
> int i = c.find("=");
> if (i < 0) return false;
> int l = c.length();
> dest = c.substr(i+1, l-i);
> trim(dest);
> return true;
> }
>
>
> static bool starts_with(string c1, string c2){
> return (c1.find(c2) == 0);
> }
>
>
> string user_properties_file(){
> int myuid = getuid();
> struct passwd *mypasswd = getpwuid(myuid);
> return string(mypasswd->pw_dir)+"/.icedtea/deployment.properties";
> }
>
>
> static string main_properties_file(){
> return "/etc/.java/deployment/deployment.properties";
> }
>
> static string getDefaultJavaPropertiesFile(){
> return ICEDTEA_WEB_JRE "/lib/deployment.properties";
> }
This camel-case is out of place.
>
>
> //this is reimplemntation as icedtea-web settings do this (Also name
> of function is the same):
> //try the main file in /etc/.java/deployment
> //if found, then return this file
> //try to find setUp jre
> // if found, then try if this file exists and end
> //if no jre custom jvm is set, then tryes default jre
> // if its deploy file exists, then return
> //not dound otherwise
> bool find_system_config_file(string& dest){
> if(access(main_properties_file().c_str(), F_OK ) != -1 ) {
Please create a utility function that attempts to open a file and sees
if it is NULL (and closes it if it does open it) to emulate this behaviour.
> dest = main_properties_file();
> return true;
> } else {
> string jdest;
> bool found = find_custom_jre(jdest);
> if (found){
> jdest = jdest + "/lib/deployment.properties";
> if(access(jdest.c_str(), F_OK ) != -1 ) {
> dest = jdest;
> return true;
> }
> } else {
> if(access(getDefaultJavaPropertiesFile().c_str(), F_OK )
> != -1 ) {
> dest = getDefaultJavaPropertiesFile();
> return true;
> }
> }
> }
> return false; //nothing of above found
> }
>
> //returns false if not found, true otherwise
> static bool find_property(string fileName, string property, string&
> dest){
Nit: Don't use camel-case.
Note that you can still take eg filename as a const char* if it makes
your code cleaner. You do not need to use std::string strictly if you
are taking an unchanged string parameter.
> string nwproperty(property);
> trim(nwproperty);
> nwproperty=nwproperty+"=";
> FILE *file = fopen ( fileName.c_str(), "r" );
> if ( file != NULL ){
> char line [ 512 ]; /* or other suitable maximum line size */
I already gave you a solution that does not require a static size buffer.
> while ( fgets ( line, sizeof line, file ) != NULL ){ /* read a
> line */
> string copy = string(line);
> //java tolerates spaces arround = char, remove them for
> matching
s/arround/around/
> remove_all_spaces(copy);
> //printf(line);
> //printf(copy.c_str());
> if (starts_with(copy,nwproperty)) {
> fclose (file);
> //provide non-spaced value, triming is done in
> get_property_value
s/triming/trimming/
> get_property_value(copy, dest);
> return true;
> }
> }
>
> }else{
> perror(fileName.c_str()); /* why didn't the file open? */
> }
> return false;
> }
>
>
> //this is reimplmentation of itw-settings operations
> //first check in user's settings, if found, return
> //then check in global file (see the magic of find_system_config_file)
> bool read_deploy_property_value(string property, string& dest){
> //is it in user's file?
> string keeper;
> bool a = find_property(user_properties_file(), property, keeper);
> if (a) {
> dest=keeper;
> return true;
> }
> //is it in global file?
> bool b = find_system_config_file(keeper);
> if (b) {
> return find_property(keeper, property, dest);
> }
> return false;
> }
>
> //This is different from common get property, as it is avoiding to
> search in *java*
> //properties files
> bool find_custom_jre(string& dest){
> string key("deployment.jre.dir");
It is fine to just use a const char* here (and have find_property take a
const char*). std::string is mainly a benefit for mutable strings. It's
correct, at least.
> string keeper;
> if(access(user_properties_file().c_str(), F_OK ) != -1 ) {
> bool a = find_property(user_properties_file(),key, keeper);
Nice variable name :-) ... you can embed this right in the if statement
if you don't have a good name for it.
> if (a) {
> dest = keeper;
Pass 'dest' instead of 'keeper' to find_property and this will not be
required.
> return true;
> }
> }
> if(access(main_properties_file().c_str(), F_OK ) != -1 ) {
> return find_property(main_properties_file(),key, keeper);
Bug: this does not modify 'dest'. Pass 'dest' instead of 'keeper'.
> }
> return false;
> }
>
>
>
> int main(void){
> printf("user's settings file\n");
> cout << user_properties_file();
> printf("\nmain settings file:\n");
> cout << (main_properties_file());
> printf("\njava settings file \n");
> cout << (getDefaultJavaPropertiesFile());
> printf("\nsystem config file\n");
> string a1;
> find_system_config_file(a1);
> cout << a1;
> printf("\ncustom jre\n");
> string a2;
> find_custom_jre(a2);
> cout << a2;
> printf("\nsome custom property\n");
> string a3;
> read_deploy_property_value("deployment.security.level", a3);
> cout << a3;
> printf("\n");
> return 0;
> }
Not too bad :-)
Keep at it! Just friendly review from intern etc etc
Happy hacking,
-Adam
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130318/204cb080/attachment.html
More information about the distro-pkg-dev
mailing list