[rfc][icedtea-web] read properties values from C part - library edition :)
Jiri Vanek
jvanek at redhat.com
Wed Mar 20 07:19:36 PDT 2013
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 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 ?)
Still keeping the original name convention but the refactoring is worthy to think about.
>
> re unit tests:
> Examples can be found in tests/cpp-unit-tests.
AAA! Thanx so much for example. I will do the tests in next round!
>
> 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 <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 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.
Hmm. Replaced by your suggested version... But..:)
I have kept the status get the variable, modify it, and return it. I like the possibility of
chaining... If you really do not like it I will fix with deepest sorrow in integration round:((
All the minors below should be fixed.
>
>>
>>
>>
>>
>> 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 --------------
#include <string>
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
//testing api
const string default_file_name = "deployment.properties";
const string custom_jre_key = "deployment.jre.dir";
string trim(string& str);
void remove_all_spaces(string& str);
bool get_property_value(string c, string& dest);
bool starts_with(string c1, string c2);
bool file_exists(string filename);
string user_properties_file();
string main_properties_file();
string default_java_properties_file();
//for passing three dummy files
bool find_system_config_file(string f1, string f2, bool usef2, string f3, string& dest);
bool find_property(string filename, string property, string& dest);
//for passing two dummy files
bool read_deploy_property_value(string f1, string f2, bool usef2, string property, string& dest);
//for passing two dummy files
bool find_custom_jre(string f1, string f2,string& dest);
//end of testing api
-------------- next part --------------
#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;
}
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){
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());
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
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){
//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){
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;
}
More information about the distro-pkg-dev
mailing list