[rfc][icedtea-web] read properties values from C part - library edition :)
Adam Domurad
adomurad at redhat.com
Fri Feb 22 10:01:56 PST 2013
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++.
Happy hacking,
-Adam
More information about the distro-pkg-dev
mailing list