[rfc][icedtea-web] read properties values from C part - library edition :)

[Adam Domurad]

On 03/20/2013 10:19 AM, Jiri Vanek wrote:
> On 03/19/2013 03:44 PM, Adam Domurad wrote:
>> On 03/19/2013 09:41 AM, Jiri Vanek wrote:
>>> 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.
>>
>> Doesn't look half bad this time :-) Feel free to propose an 
>> integrated version.
> One more round rather...
> I have made the header much bigger - for testing purposes. I have some 
> memory that all exported (testable) functions must be in header file...

Note: You do not need these in header, technically. It may be cleaner to 
not expose these normally and include the lines directly in the test, ie:

string trim(string& str);
void remove_all_spaces(string& str);

SUITE (...) {
   /// etc
}

TYVM for reorganizing the code to be testable! I know the elegance is 
hard to part with.


[.. rest snipped ..]


> #include <unistd.h>
> #include <sys/types.h>
> #include <pwd.h>
> #include <cstdio>
> #include <cstdlib>
> #include <string>
> #include <functional>
> #include <cctype>
> #include <locale>
> #include <iostream>
> #include <fstream>
> #include "IcedTeaParsePropeties.h"
>
> using namespace std;
>
> string trim(string& str) {
>     size_t start = str.find_first_not_of(" \t\n"), end = 
> str.find_last_not_of(" \t\n");
>     if (start == std::string::npos) {
>             return str;
>     }
>     str = str.substr(start, end - start + 1);
>     return str;
> }

I don't see how this supports chaining (it returns only a copy). Also 
you don't use this string value anywhere, why not just 'void'?
I'm in favour of moving this into IcedTeaPluginUtils and dropping the 
return value.

>
>
>
>
> void remove_all_spaces(string& str)
> {
>     for(int i=0; i<str.length(); i++){
>         if(str[i] == ' '  || str[i] == '\n' || str[i] == '\t') {
>             str.erase(i,1);
>         }
>     }
> }
>
> 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;
> }
>
>
> bool starts_with(string c1, string c2){
>         return (c1.find(c2) == 0);
> }
>
> bool file_exists(string filename)
> {
>     ifstream infile(filename.c_str());
>     return infile.good();
> }
>
>
> string  user_properties_file(){
>     int myuid = getuid();
>     struct passwd *mypasswd = getpwuid(myuid);
>     return string(mypasswd->pw_dir)+"/.icedtea/"+default_file_name;
> }
>
>
> string  main_properties_file(){
>     return "/etc/.java/deployment/"+default_file_name;
> }
>
> string default_java_properties_file(){
>     return  ICEDTEA_WEB_JRE "/lib/"+default_file_name;
> }
>
>
> /* this is the same search done by icedtea-web settings:
>    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 tries default jre
>    if its  deploy file exists, then return
>    not found otherwise*/
> bool find_system_config_file(string& dest){
>     string jdest;
>     bool found = find_custom_jre(jdest);
>     return find_system_config_file(main_properties_file(), jdest, 
> found, default_java_properties_file(), dest);
> }
> bool find_system_config_file(string f1, string f2, bool usef2, string 
> f3, string& dest){

f1, f2, f3 -> user_file, global_file, default_file please. Also worth 
considering making the 2nd string nullable, ie using a string* (note 
this won't be as painful as returning string*, since you can safely pass 
with & operator here).

>     if (file_exists(f1)) {
>         dest = f1;
>         return true;
>     } else {
>         if (usef2){
>             string jdest = f2 + "/lib/"+default_file_name;
>             if(file_exists(jdest) ) {
>                 dest = jdest;
>                 return true;
>             }
>         } else {
>             if(file_exists(f3)) {
>             dest = f3;
>             return true;
>             }
>         }
>     }
> return false; //nothing of above found
> }
>
> //Returns whether property was found, if found stores result in 'dest'
> bool find_property(string filename, string property, string& dest){
>     string  property_matcher(property);
>     trim( property_matcher);
>      property_matcher= property_matcher+"=";
>     ifstream input( filename.c_str() );
>     for( string line; getline( input, line ); ){ /* read a line */
>         string copy = line;
>         //java tolerates spaces around = char, remove them for matching
>         remove_all_spaces(copy);
>         //printf(line.c_str());
>         //printf(copy.c_str());

[nit] remove these commented lines

>         if (starts_with(copy, property_matcher)) {
>             //provide non-spaced value, trimming is  done in 
> get_property_value
>             get_property_value(line, dest);
>             return true;
>             }
>         }
>
>     return false;
>     }
>
>
> /* this is reimplmentation of itw-settings operations

OK. First time you typo something, turn on some spellcheck >:).
s/reimplmentation/reimplementation/ ... again

>    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){
>     string futurefile;
>     bool b = find_system_config_file(futurefile);
>     return read_deploy_property_value(user_properties_file(), 
> futurefile, b, property, dest);
> }
> bool  read_deploy_property_value(string f1, string f2, bool usef2, 
> string property, string& dest){

f1, f2 -> user_file, global_file please. Consider making global_file a 
nullable string*.

>     //is it in user's file?
>     bool found = find_property(f1, property, dest);
>     if (found) {
>         return true;
>     }
>     //is it in global file?
>     if (usef2) {
>         return find_property(f2, 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){
>     return find_custom_jre(user_properties_file(), 
> main_properties_file(), dest);
> }
> bool  find_custom_jre(string f1, string f2,string& dest){

f1, f2 -> user_file, global_file please

>     string key = custom_jre_key;
>     if(file_exists(f1)) {
>         bool a = find_property(f1, key, dest);
>         if (a) {
>             return true;
>         }
>     }
>     if(file_exists(f2)) {
>         return find_property(f2, key, dest);
>     }
> 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 << (default_java_properties_file());
>     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;
> }

Looks good otherwise, go ahead and start testing+integration if you'd like.
-Adam