[rfc][icedtea-web] read properties values from C part - library edition :)
Jiri Vanek
jvanek at redhat.com
Tue Mar 19 06:41:28 PDT 2013
So another round. I think all is fixed excet the c x c+++ headers. Do you mind t write little bit more what you need from me?
Also unittests will be needed. I would like to wrote them, ..will need probably some code snippet and general advices from you when I will integrate (next round?) this "library" inside ITW.
Thanx for patient and detailed review!
J.
On 03/18/2013 08:09 PM, Adam Domurad wrote:
> 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 --------------
A non-text attachment was scrubbed...
Name: parsePropeties.cc
Type: text/x-csrc
Size: 4896 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130319/39178a78/parsePropeties.cc
More information about the distro-pkg-dev
mailing list