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

[Adam Domurad]

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.
Note that it seems the naming scheme for the CPP side is *.cc files 
using camelcase & starting capital, so name it ParseProperties.cc, and 
give a header called ParseProperties.h.
(Side note:   Actually, they all have the form IcedTea*.cc, but I think 
this is too noisy. I'd be in favour of removing this prefix for all the 
files actually, any thoughts on this ?)

re unit tests:
Examples can be found in tests/cpp-unit-tests.

Steps:
1. Create a file called ParsePropertiesTest.cc
2. Include ParseProperties.h
3. Use the SUITE and TEST macros. Here's a suggested layout (Disclaimer: 
I *did not* compile this):

#include <cstdio>
#include "ParseProperties.h"

/* Creates a temporary file with the specified contents */
static std::string temporary_file(const std::string& contents) {
    std::string path = *tmpnam(NULL);* /* POSIX function, fine for test 
suite */
    fstream file(path.c_str(), fstream::in);
    file << contents;
    return path;
}

SUITE(ParseProperties) {

     TEST(read_deploy_property_value) {
         std::string value;
         bool found = read_deploy_property_value("test", value);
         CHECK(found);
         CHECK(value == "foobar");
     }
}

Although after writing this it occurred to me it is a bit tricky to read 
from a dummy file. Possibly you want to design some way to direct these 
functions to read dummy files. Other options, back up these files and 
replace them (dangerous), use some chroot tricks? (never tried).
Anyway feel free to ask more questions, I think the concept is simple. 
Once you have a .cc file in the test directory, and the TEST macro, the 
test system will pick up all your TEST's.

>
> Thanx for patient and detailed review!
>
> J.
>
> [..snip..]

re new version:

> #include <unistd.h>
> #include <sys/types.h>
> #include <pwd.h>
> #include <stdio.h>
> #include <stdlib.h>

#include <stdio.h> -> #include <cstdio>
#include <stdlib.h> -> #include <cstdlib>

I suppose it is easier if I just tell you what to do :-) Not a huge 
point anyway.

> #include <string>
> #include <algorithm>
> #include <functional>
> #include <cctype>
> #include <locale>
> #include <iostream>
> #include<fstream>
>
> 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

Just a note, this will be extracted to header file. Maybe for next 
iteration you should do this (you can test that the header works with a 
test main in a separate .cpp file)

>
>
> // trim from start
> static string &ltrim(string& s) {
>         s.erase(s.begin(), find_if(s.begin(), s.end(), 
> not1(ptr_fun<int, int>(isspace))));
>         return s;
> }
> // trim from end
> static 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 string &trim(string& s) {
>         return ltrim(rtrim(s));
> }

Ok you don't need to use my version but IMO you must:
1. Make this more readable. It is a simple operation, I would rather not 
use the rather complex templated algorithm & functional library in C++
2. Either return 'string' and take 'string' (not 'string&') OR take 
'string&' but do not return it. Returning a string makes it seem like 
the original string is unchanged.

>
>
>
>
> static 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);
>         }
>     }
> }
>
> 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);
> }
>
> static bool file_exists(string filename)
> {
>     ifstream infile(filename.c_str());
>     return infile.good();
> }

This is good, thanks.

>
>
> 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 default_java_properties_file(){
>     return  ICEDTEA_WEB_JRE "/lib/deployment.properties";
> }
>
>
> //this is reimplemntation as icedtea-web settings do this (Also name 
> of function is the same):

s/ reimplemntation / reimplementation /
May be better worded as eg '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 tryes default jre

s/tryes/tries/

> //  if its  deploy file exists, then return
> //not dound otherwise

s/dound/found/
[Nit] Could you put the whole thing in a /**/ quote ?

> bool find_system_config_file(string& dest){
>     if(file_exists(main_properties_file())) {
>         dest = main_properties_file();
>         return true;
>     } else {
>         string jdest;
>         bool found = find_custom_jre(jdest);
>         if (found){
>             jdest = jdest + "/lib/deployment.properties";
>             if(file_exists(jdest) ) {
>                 dest = jdest;
>                 return true;
>             }
>         } else {
>             if(file_exists(default_java_properties_file())) {
>             dest = default_java_properties_file();
>             return true;
>             }
>         }
>     }
> return false; //nothing of above found
> }
>
> //returns false if not found, true otherwise

[nit] "Returns whether property was found, if found stores result in 
'dest'" is more descriptive.

> static bool  find_property(string filename, string property, string& 
> dest){
>     string nwproperty(property);
>     trim(nwproperty);
>     nwproperty=nwproperty+"=";

[nit] It'd be clearer if this was something like string property_matcher 
= trim(property) + "="; (assuming a trim that returns a copy))

>     ifstream input( filename.c_str() );
>     for( string line; getline( input, line ); ){ /* read a line */
>         string copy = string(line);

string(line) is redundant.

>         //java tolerates spaces around = char, remove them for matching
>         remove_all_spaces(copy);
>         //printf(line.c_str());
>         //printf(copy.c_str());

Remember to drop these.

>         if (starts_with(copy,nwproperty)) {
>             //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

s/ reimplemntation / reimplementation /
May be better worded as eg 'this is the same search done by icedtea-web 
settings'

> //first check in user's settings, if found, return
> //then check in global file (see the magic of find_system_config_file)

[nit] I'd prefer a /**/ quote

> bool  read_deploy_property_value(string property, string& dest){
>     //is it in user's file?
>     bool a = find_property(user_properties_file(), property, dest);
>     if (a) {
>         return true;
>     }
>     //is it in global file?
>     string futurefile;
>     bool b = find_system_config_file(futurefile);
>     if (b) {
>         return find_property(futurefile, 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");
>     if(file_exists(user_properties_file())) {
>         bool a = find_property(user_properties_file(),key, dest);

[nit] I still do not like this 'a' variable. (it makes some sense in 
read_deploy_property_value, but not here)

>         if (a) {
>             return true;
>         }
>     }
>     if(file_exists(main_properties_file())) {
>         return find_property(main_properties_file(),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;
> }

Good work!
-Adam
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130319/f831e455/attachment.html