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

Jiri Vanek jvanek at redhat.com
Sun Feb 24 07:32:33 PST 2013


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.
>
>> #include <string.h>
>
> In C++, you should use #include <cstring>, #include <cstdio>, #include <cstdlib> -- pretty much no '.h' for the standard library.
>
>>
>> //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:
>
> 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:
>
> 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.
>
>> }
>>
>> 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.
>
>> 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.
>
>> 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).
>
>>
>>
>> //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.



More information about the distro-pkg-dev mailing list