[rfc][icedtea-web] read properties values from C part - library edition :)
Jiri Vanek
jvanek at redhat.com
Mon Mar 18 10:24:35 PDT 2013
On 02/24/2013 04:47 PM, Jiri Vanek wrote:
> On 02/24/2013 04:32 PM, Jiri Vanek wrote:
>> On 02/22/2013 07:01 PM, Adam Domurad wrote:
>>> On 02/22/2013 08:40 AM, Jiri Vanek wrote:
>>>> Replacing harcoded jre path - part 3, intermezzo.
>>>>
>>>> This code should allow to parse properties from plugin side. As I can not find jvm by launching
>>>> jvm O:) - as Saad did.
>>>>
>>>> Please be kind to me, I'm removing dust from my C coding just slowly ;)
>>>>
>>>> Later the empty getPluginExecutbale and getPluginRtJar from "[rfc][icedtea-web] replacing of
>>>> hardcoded jre path by function" should call the findCustomJre and provide its value(if found)
>>>> instead of default one (but still provide default as fallback)
>>>>
>>>> I will probably also substitute Saad's code (which is launching jvm to get property. It is
>>>> significant performance drop) with this new getDeployProperty method later.
>>>>
>>>> J.
>>>>
>>>> ps: this is part of make-jredir-configurable after install effort
>>>
>>> I take it from our IRC discussion that you mixed up the fact that 0 is the only false value /
>>> everything else is true ?
>>> I'll comment mostly on best practice for now :-)
>>>
>>> First of all, function naming convention on our C++ side is 'this_is_a_function'. Class methods
>>> remain camelCase. It not bad once you are use to it :-)
>>> Functions named eg 'getDefaultSomething()' are as well not as common, particularly in top level
>>> functions. More common is just 'default_something()'
>>>
>>> However I am quite happy that you are dusting off your C :-)
>>>
>>>> #include <unistd.h>
>>>> #include <sys/types.h>
>>>> #include <pwd.h>
>>>> #include <stdio.h>
>>>> #include <stdlib.h>
>>>> #include <wordexp.h>
>>>
>>> Actually I've never seen this wordexp header. I don't think we should use GNUisms if we don't
>>> need to.
removed and repalced by better one
>>>
>>>> #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.
>>>
>>>>
>>>> //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 :((
>>>
>>> std::string str = <whatever>;
>>> int x = <whatever>;
>>> str.erase( x, 1 );
>>>
>>>> if (i > x){
>>>> c[i-1] = c[i];
>>>> }
>>>> }
>>>> }
>>>>
>>>> static void removeSpaces(char *c){
>>>
>>>
>>>> int i;
>>>> for ( i = 0; i < strlen(c); i++){
>>>> if ( c[i] == ' ') {
>>>> popChar(c,i);
>>>> i--;
>>>> }
>>>> }
>>>> }
>>>>
>>>> static void removeStartSpaces(char *c){
>>>> int i;
>>>> for ( i = 0; i < strlen(c); i++){
>>>> if ( c[i] == ' ' || c[i] == '\n' ) {
>>>> popChar(c,i);
>>>> i--;
>>>> } else return;
>>>> }
>>>> }
>>>>
>>>> static void removeTailSpaces(char *c){
>>>> int i;
>>>> for ( i = strlen(c)-1 ; i >- 0; i--){
>>>> if ( c[i] == ' ' || c[i] == '\n' ) {
>>>> popChar(c,i);
>>>> } else return;
>>>> }
>>>> }
>>>>
>>>> static void trim(char *c){
>>>> removeTailSpaces(c);
>>>> removeStartSpaces(c);
>>>> }
>>>>
>>>> static void clean(char *c){
>>>
>>> Whats funny is that in this function, it will set c[0] to '\0' and then strlen will return 0 and
>>> it will exit.
>>> Anyway, for std::string you have your choice of string.clear(), or string = "";
>>>
>>>> int i;
>>>> for ( i = 0; i < strlen(c); i++){
>>>> c[i] = '\0';
>>>> }
>>>> }
>>>>
>>>>
>>>> static int equalsPos(char *c){
>>>> int i;
>>>> for ( i = 0; i < strlen(c); i++){
>>>> if ( c[i] == '=') {
>>>> return i;
>>>> }
>>>> }
>>>> return -1;
>>>> }
>>>>
>>>> static char* getPropertyValue(char* c, char* dest){
>>>> int i = equalsPos(c);
>>>> if (i == -1) c;
>>>
>>> ^ This line does nothing :-))
>>>
>>>> int l = strlen(c);
>>>> strncpy(dest, c+i+1, l-i);
>>>> trim(dest);
>>>> return dest;
>>>> }
>>>>
>>>>
>>>> static int startsWith(char *c1, char* c2){
>>>
>>> Even in plain C you can do strncmp(c1, c2, strlen(c2) ) to achieve this.
>>>
>>> With std::string I still like to use strncmp as follows:
>>> std::string str = <whatever>;
>>> strncmp( str.c_str(), str2.c_str(), str2.size() );
>>>
>>>> int i;
>>>> int l1 = strlen(c1);
>>>> int l2 = strlen(c2);
>>>> int min = l1;
>>>> if (l1 > l2) min = l2;
>>>> for ( i = 0; i < min; i++){
>>>> if ( c1[i] != c2[i]) {
>>>> return 1; //fasle
>>>
>>> :-)) Use bool, true & false.
>>>
>>>> }
>>>> }
>>>> return 0;//true
>>>> }
>>>>
>>>>
>>>> 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.
>>>
>>> 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 :(
>>>
>>>> }
>>>>
>>>> static char * getDefaultJavaPropertiesFile(){
>>>> return ICEDTEA_WEB_JRE "/lib/deployment.properties";
>>>> }
>>>>
>>>>
>>>> //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
>>>> int findSystemConfigFile(char* dest){
>>>> if(access(getMainPropertiesFile(), F_OK ) != -1 ) {
>>>> strcpy(dest, getMainPropertiesFile());
>>>> return 0;//file found
>>>> } else {
>>>> char jDest[512];
>>>
>>> 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?
>>>
>>>> strcpy(dest, getDefaultJavaPropertiesFile());
>>>> return 0; //file found
>>>> }
>>>> }
>>>> }
>>>> return 1; //nothing of above found
>>>> }
>>>>
>>>> //returns true if found, false otherwise
>>>> static int findProperty(char* fileName, char* property, char* dest){
>>>> char nwprpty[strlen(property) + 2];
>>>
>>> This is C99 only! This is not valid in C89 or C++. You may only specify constants for static
>>> arrays (use std::string, etc, etc).
>>>
>>>> strcpy(nwprpty, property);
>>>> strcat(nwprpty, "=");
>>>> FILE *file = fopen ( fileName, "r" );
>>>> if ( file != NULL ){
>>>> char line [ 512 ]; /* or other suitable maximum line size */
>>>> while ( fgets ( line, sizeof line, file ) != NULL ){ /* read a line */
>>>> char copy[512];
>>>> strcpy(copy, line);
>>>> //java tolerates sapces arround = char, remove them for matching
>>>> removeSpaces(copy);
>>>
>>> IMO we should match the property name, skip to the character just past this property, loop until
>>> we hit the = sign, loop until we hit a non-space character.
>>>
>>>> //printf(line);
>>>> //printf(copy);
>>>> if (startsWith(copy,nwprpty) == 0) {
>>>> fclose (file);
>>>> //provide non-sapced value, triming is done in getPropertyValue
>>>> getPropertyValue(line,dest);
>>>> return 0;//found!
>>>> }
>>>> }
>>>>
>>>> }else{
>>>> perror (fileName); /* why didn't the file open? */
>>>> }
>>>> return 1;//not found:(
>>>> }
>>>
>>> Something like:
>>>
>>> static bool find_property(const char* filename, const char* property, std::string& dest) {
>>> std::fstream file(filename, std::fstream::in);
>>>
>>> if (!file) {
>>> ...
>>> }
>>>
>>> std::string line;
>>> while (!file.eof()) {
>>> std::getline(file, line);
>>> ...
>>> }
>>>
>>> return false; // Not found
>>> }
>>>
>>> , would be more idiomatic.
>>>
>>> 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?
>>>>
>>>>
>>>> //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 findSystemConfigFile)
>>>> int getDeployProperty(char* property, char* dest){
>>>> //is it in user's file?
>>>> int a = findProperty(getUserPropertiesFile(), property, dest);
>>>> if (a == 0) return 0;
>>>> //is it in global file?
>>>> char file[512];
>>>> int b = findSystemConfigFile(file);
>>>> if (b == 0) {
>>>> return findProperty(file, property, dest);
>>>> }
>>>> return 1;
>>>> }
>>>>
>>>> //This is different from common get property, as it is avoiding to search in *java*
>>>> //properties files
>>>> int findCustomJre(char* dest){
>>>> char* key = "deployment.jre.dir";
>>>> int a = findProperty(getUserPropertiesFile(),key, dest);
>>>> if (a == 0) return 0;
>>>> int b = findProperty(getMainPropertiesFile(),key, dest);
>>>> return b;
>>>>
>>>> }
>>>>
>>>>
>>>>
>>>> int main(void){
>>>> printf(getUserPropertiesFile());
>>>> printf("\n");
>>>> printf(getMainPropertiesFile());
>>>> printf("\n");
>>>> printf(getDefaultJavaPropertiesFile());
>>>> printf("\n");
>>>> char dest1[512];
>>>> clean(dest1);
>>>> int i1 = findSystemConfigFile(dest1);
>>>> printf(dest1);
>>>> printf("\n");
>>>> char dest2[512];
>>>> clean(dest2);
>>>> int i2 = findCustomJre(dest2);
>>>> printf(dest2);
>>>> printf("\n");
>>>> char dest3[512];
>>>> clean(dest3);
>>>> int i3 = getDeployProperty("deployment.security.level", dest3);
>>>> printf(dest3);
>>>> printf("\n");
>>>> return 0;
>>>> }
>>>
>>>> OK enough nits. Glad you are dusting off your C -- next dust off some C++ please :-) I already
>>>> am looking to eradicate the existing string fiddling in ITW, don't introduce more :-).
>>>> Anyway, see my comments in other review, we may want to go a simpler route of having the JRE/JRE
>>>> arguments just sitting in a file. But -- it does make sense to have them in deployment
>>>> properties, so if we can get a small, tested, C++ implementation running I'm fine with it.
>>>>
>>>> I can do the testing if you like once you're done, but the C++ unit tests really aren't any
>>>> harder than writing C++.
>>>
>> >
>> >> static char* getPropertyValue(char* c, char* dest)
>> >> > static int findProperty(char* fileName, char* property, char* dest)
>> >> Please consinder that these functions have to handle u encoded and any other escape character
>> encoded property names and values correctly.
>> >> Trimming off white spaces at the end of values does not comply with the property file
>> specification. Just consider paths to files or folders that have trailing white spaces in their
>> names. In this case getPropertyValue() will return wrong values.
>>
> Hi!
>
> I'm aware of most of the stuff you are describing. I'm also aware that there can be different
> delimiter (instead of = char). But:
> My implementation do not need this right now, as C code should read just path to jre - which
> should be "strange characters free" and on one line, and Launching params of applet's jvm - with
> same predicate.
> Also itw-settings is using the equals char to store the properties.
>
> Well the triming is discutable - that is what I'm not aware.... I thought tha "KEY= value " should
> be evaluated at least to to "value ". And I thought also about full trimming (wrongly appearently)
> I would like to let this as subject to further discussion - I'm not against abandoning the trimming.
> The discussion should be focussed on - Is bigger evil to trim, and so fix accidentally wrongly
> typed path, or not trim and so expect path with spaces at the end?
> Imho the spaces at the end of jre dir, however correct by OS possibilities, ar unlikely to appear.
>
> In case that native plugin will go beyond the reading of those two "commandline" properties, then
> I would like to abandon my custom code, and to find some more suitable library.
>
> I hope my reply have made you little bit more optimistic :)
>
> Thanx for opinions,
> J.
>
Ok, here we go with c++, string version....
/me terrified
-------------- next part --------------
#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));
}
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);
}
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 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 ) {
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){
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 */
while ( fgets ( line, sizeof line, file ) != NULL ){ /* read a line */
string copy = string(line);
//java tolerates spaces arround = char, remove them for matching
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
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");
string keeper;
if(access(user_properties_file().c_str(), F_OK ) != -1 ) {
bool a = find_property(user_properties_file(),key, keeper);
if (a) {
dest = keeper;
return true;
}
}
if(access(main_properties_file().c_str(), F_OK ) != -1 ) {
return find_property(main_properties_file(),key, 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;
}
More information about the distro-pkg-dev
mailing list