<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">Thanks for sticking with this :-)<br>
      <br>
      On 03/18/2013 01:24 PM, Jiri Vanek wrote:<br>
    </div>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">[..snip..]<br>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <br>
            <blockquote type="cite">#include &lt;string.h&gt;
              <br>
            </blockquote>
            <br>
            In C++, you should use #include &lt;cstring&gt;, #include
            &lt;cstdio&gt;, #include &lt;cstdlib&gt; -- pretty much
            <br>
            no '.h' for the standard libra
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      ugh. I'm afraid I need more hints what to replace with what and
      what will be impact to the code:(
      <br>
      ry.
      <br>
    </blockquote>
    <br>
    No problem. GCC will accept both, but you should use 'c' to prefix
    standard C library headers in C++. It's part of the C++ standard. (I
    don't think we should do something non-compliant just because the
    compiler lets us)<br>
    <br>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <br>
            <blockquote type="cite">
              <br>
              //public api
              <br>
              char* getUserPropertiesFile();
              <br>
              int findSystemConfigFile(char* dest);
              <br>
              int findCustomJre(char* dest);
              <br>
              int getDeployProperty(char* property, char* dest);
              <br>
              //end of public api
              <br>
              <br>
              static void popChar(char *c, int x){
              <br>
              int i;
              <br>
              for ( i = x; i &lt;= strlen(c); i++){//moving also \0
              <br>
            </blockquote>
            <br>
            'strlen' is a relatively expensive operation and should not
            be used in a loop condition.
            <br>
            This is more of a 'delete' than a pop.
            <br>
            <br>
            I strongly recommend using std::string, which defines this,
            eg:
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      done, but teh benefit is lesser then expected :((
      <br>
    </blockquote>
    <br>
    Using char* by itself is very bug-prone; I see plenty benefit.<br>
    <br>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">
      <br>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <blockquote type="cite">
              <br>
              <br>
              char* getUserPropertiesFile(){
              <br>
              struct passwd *pw = getpwuid(getuid());
              <br>
              wordexp_t exp_result;
              <br>
              wordexp("~/.icedtea/deployment.properties",
              &amp;exp_result, 0);
              <br>
              return exp_result.we_wordv[0];
              <br>
            </blockquote>
            <br>
            I would like to avoid using these GNU specific functions if
            possible.
            <br>
            This has the potential to leak memory as wordfree is never
            called.
            <br>
            We can instead use the standard function getenv("HOME") like
            so:
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      nope, home can not be defined. It is not an rule. i have used
      standard function reading from psswd.
      <br>
    </blockquote>
    <br>
    This is a *POSIX function*, not a *standard C/C++ function* :-).<br>
    Anyway, I am fine with this because it truly is architecture
    specific.<br>
    <br>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <br>
            std::string user_properties_file =
            std::string(getenv("HOME")) +
            /.icedtea/deployment.properties";
            <br>
            Note for '+' to work properly on C++ strings, one side of
            the expression must be an std::string.
            <br>
            (Similar to Java -- except unfortunately string constants
            are not std::string's in C++)
            <br>
            <br>
            You can also check if the returned environment variable is
            empty as follows:
            <br>
            <br>
            std::string homedir = getenv("HOME");
            <br>
            if (homedir.empty()) {
            <br>
            ... fallback code ... (concatenate HOMEDRIVE and HOMEPATH
            environment variables if you want to
            <br>
            pretend we support Windows :-)
            <br>
            }
            <br>
            std::string user_properties_file = homedir +
            "/.icedtea/deployment.properties"
            <br>
            <br>
            <blockquote type="cite">}
              <br>
              <br>
              <br>
              static char* getMainPropertiesFile(){
              <br>
              return "/etc/.java/deployment/deployment.properties";
              <br>
            </blockquote>
            <br>
            const char* should always be used for strings that should
            not be modified. This is in fact not
            <br>
            valid C++ as-is.
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      as SString have come to this place, I hope it is better now :(
      <br>
    </blockquote>
    <br>
    yes<br>
    <br>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <br>
            [..snip..]<br>
            Bad Jiri! This is 2013, don't use fixed size arrays! :-)
            <br>
            std::string will make your life easier.
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      have not, but is done :)
      <br>
    </blockquote>
    <br>
    :)<br>
    <br>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            <br>
            <blockquote type="cite">int customJre =
              findCustomJre(jDest);
              <br>
              if (customJre == 0){
              <br>
              strncat(jDest,"/lib/deployment.properties",50);
              <br>
            </blockquote>
            <br>
            It is good that you're using strncat, but you're just
            guessing a size.
            <br>
            std::string will make your life easier.
            <br>
            <br>
            <blockquote type="cite">if(access(jDest, F_OK ) != -1 ) {
              <br>
              strcpy(dest, jDest);
              <br>
              return 0; //file found
              <br>
              }
              <br>
              } else {
              <br>
              if(access(getDefaultJavaPropertiesFile(), F_OK ) != -1 ) {
              <br>
            </blockquote>
            <br>
            'access' is (unfortunately) not a standard function. More
            idiomatic is to try and open the file
            <br>
            for reading and see if we get NULL back, or in the case of
            an fstream, if the fstream is empty.
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      hmm.. I let it in, I like the function. .What is th ereason for
      not using it?
      <br>
    </blockquote>
    <br>
    It is not a standard C/C++ function, it's a POSIX function. While we
    don't support non-POSIX platforms as a target officially, please
    just check if FILE* is null or if the fstream is empty when opened.
    <br>
    <br>
    <blockquote cite="mid:51474DD3.7080003@redhat.com" type="cite">
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite">
            Note that const char* is frequently passed as a parameter in
            C++ because it can be both an
            <br>
            std::string (via c_str()) or a string literal.
            <br>
            Note that fstream needs no close call at all, it is cleaned
            up once it goes out of scope -- this
            <br>
            is IMHO one of the strongest features of C++ (its resource
            management is handled exactly like its
            <br>
            memory management).
            <br>
            <br>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
      Is it still valid?
      <br>
      <br>
      Ok, here we go with c++, string version....
      <br>
      <br>
      /me terrified    <br>
    </blockquote>
    <br>
    Terrified of what ? A friendly patch review from an intern ? :-)<br>
    <br>
    <blockquote type="cite">#include &lt;unistd.h&gt;<br>
      #include &lt;sys/types.h&gt;<br>
      #include &lt;pwd.h&gt;<br>
      #include &lt;stdio.h&gt;<br>
      #include &lt;stdlib.h&gt;<br>
      #include &lt;string&gt;<br>
      #include &lt;algorithm&gt; <br>
      #include &lt;functional&gt; <br>
      #include &lt;cctype&gt;<br>
      #include &lt;locale&gt;<br>
      #include &lt;iostream&gt;<br>
      <br>
      using namespace std;<br>
      <br>
      //public api<br>
      string  user_properties_file();<br>
      bool  find_system_config_file(string&amp; dest);<br>
      bool  find_custom_jre(string&amp; dest);<br>
      bool  read_deploy_property_value(string property, string&amp;
      dest);<br>
      //end of public api<br>
      <br>
      <br>
      // trim from start<br>
      static inline string &amp;ltrim(string &amp;s) {<br>
              s.erase(s.begin(), find_if(s.begin(), s.end(),
      not1(ptr_fun&lt;int, int&gt;(isspace))));<br>
              return s;<br>
      }<br>
      // trim from end<br>
      static inline string &amp;rtrim(string &amp;s) {<br>
              s.erase(find_if(s.rbegin(), s.rend(), not1(ptr_fun&lt;int,
      int&gt;(isspace))).base(), s.end());<br>
              return s;<br>
      }<br>
    </blockquote>
    <blockquote type="cite">// trim from both ends<br>
      static inline string &amp;trim(string &amp;s) {<br>
              return ltrim(rtrim(s));<br>
      }<br>
    </blockquote>
    <br>
    Don't use inline here, it's not necessary or useful.<br>
    Well these are funny, they look pasted from stack overflow :-). You
    only use trim(), here's something more readable from Stackoverflow
    that doesn't need &lt;algorithm&gt;:<br>
    <pre style="" class="lang-c prettyprint prettyprinted"><code><span class="pln">std</span><span class="pun">::</span><span class="pln">string trim</span><span class="pun">(</span><span class="kwd">const</span><span class="pln"> std</span><span class="pun">::</span><span class="pln">string</span><span class="pun">&amp;</span><span class="pln"> str</span><span class="pun">)</span><span class="pun"></span><span class="pln"> </span><span class="pun">{</span><span class="pln">
    size_t start</span><span class="pln"> </span><span class="pun">=</span><span class="pln"> str</span><span class="pun">.</span><span class="pln">find_first_not_of</span><span class="pun">(</span></code><code><span class="pun"><code><span class="pln"></span><span class="str">" \t"), </span></code></span></code><code><span class="pun"><code><span class="pln">end</span><span class="pln"> </span><span class="pun">=</span><span class="pln"> str</span><span class="pun">.</span><span class="pln">find_last_not_of</span><span class="pun">(</span></code><code><span class="pun"><code><span class="pln"></span><span class="str">" \t");</span></code>
</span></code></span><span class="pln"></span><span class="pun"></span><span class="pln">
    </span><span class="kwd">if</span><span class="pln"> </span><span class="pun">(start</span><span class="pln"> </span><span class="pun">==</span><span class="pln"> std</span><span class="pun">::</span><span class="pln">string</span><span class="pun">::</span><span class="pln">npos</span><span class="pun">)</span><span class="pln"> {
        </span><span class="kwd">return</span><span class="pln"> </span><span class="str">""</span><span class="pun">;
    }</span><span class="pln"></span><span class="com"></span><span class="pln">
</span><span class="pln">
    </span><span class="kwd">return</span><span class="pln"> str</span><span class="pun">.</span><span class="pln">substr</span><span class="pun">(start</span><span class="pun">,</span><span class="pln"> end - start + 1</span><span class="pun">);</span><span class="pln">
</span><span class="pun">}
</span></code>
Why are you returning the reference? In-effect the function signature is a lie :) Either mutate it or assign a copy (like my version does)
</pre>
    <code><span class="pun">[nit] 'string&amp; s' preferably over
        'string &amp;s'. 'string&amp;' is read as the type, not part of
        the variable name.<br>
        <br>
      </span></code>
    <blockquote type="cite"><br>
      <br>
      <br>
      static void remove_all_spaces(string&amp; str)<br>
      {<br>
          for(int i=0; i&lt;str.length(); i++)<br>
          if(str[i] == ' '  || str[i] == '\n' ) str.erase(i,1);<br>
      }<br>
    </blockquote>
    <br>
    Some braces /indenting would be nice.<br>
    <br>
    <blockquote type="cite"><br>
      static bool  get_property_value(string c, string&amp; dest){<br>
          int i = c.find("=");<br>
          if (i &lt; 0) return false;<br>
          int l = c.length();<br>
          dest = c.substr(i+1, l-i);<br>
          trim(dest);<br>
          return true;<br>
      }<br>
      <br>
      <br>
      static bool starts_with(string c1, string c2){<br>
              return (c1.find(c2) == 0);<br>
      }<br>
    </blockquote>
    <blockquote type="cite"><br>
      <br>
      string  user_properties_file(){<br>
          int myuid = getuid();<br>
          struct passwd *mypasswd = getpwuid(myuid);<br>
    </blockquote>
    <blockquote type="cite">    return
      string(mypasswd-&gt;pw_dir)+"/.icedtea/deployment.properties";<br>
      }<br>
      <br>
      <br>
      static string  main_properties_file(){<br>
          return "/etc/.java/deployment/deployment.properties";<br>
      }<br>
      <br>
      static string getDefaultJavaPropertiesFile(){<br>
          return  ICEDTEA_WEB_JRE "/lib/deployment.properties";<br>
      }<br>
    </blockquote>
    <br>
    This camel-case is out of place.<br>
    <br>
    <blockquote type="cite"><br>
      <br>
      //this is reimplemntation as icedtea-web settings do this (Also
      name of function is the same):<br>
      //try  the main file in /etc/.java/deployment<br>
      //if found, then return this file<br>
      //try to find setUp jre<br>
      //  if found, then try if this file exists and end<br>
      //if no jre custom jvm is set, then tryes default jre<br>
      //  if its  deploy file exists, then return<br>
      //not dound otherwise<br>
      bool find_system_config_file(string&amp; dest){<br>
          if(access(main_properties_file().c_str(), F_OK ) != -1 ) {<br>
    </blockquote>
    <br>
    Please create a utility function that attempts to open a file and
    sees if it is NULL (and closes it if it does open it) to emulate
    this behaviour.<br>
    <br>
    <blockquote type="cite">        dest = main_properties_file();<br>
              return true;<br>
          } else {<br>
              string jdest;<br>
              bool found = find_custom_jre(jdest);<br>
              if (found){<br>
                  jdest = jdest + "/lib/deployment.properties";<br>
                  if(access(jdest.c_str(), F_OK ) != -1 ) {<br>
                      dest = jdest;<br>
                      return true;<br>
                  } <br>
              } else {<br>
                  if(access(getDefaultJavaPropertiesFile().c_str(), F_OK
      ) != -1 ) {<br>
                  dest = getDefaultJavaPropertiesFile();<br>
                  return true;<br>
                  } <br>
              }    <br>
          }<br>
      return false; //nothing of above found<br>
      }<br>
      <br>
      //returns false if not found, true otherwise<br>
      static bool  find_property(string fileName, string property,
      string&amp; dest){<br>
    </blockquote>
    <br>
    Nit:   Don't use camel-case.<br>
    Note that you can still take eg filename as a const char* if it
    makes your code cleaner. You do not need to use std::string strictly
    if you are taking an unchanged string parameter.<br>
    <br>
    <blockquote type="cite">    string nwproperty(property);<br>
          trim(nwproperty);<br>
          nwproperty=nwproperty+"=";<br>
          FILE *file = fopen ( fileName.c_str(), "r" );<br>
          if ( file != NULL ){<br>
              char line [ 512 ]; /* or other suitable maximum line size
      */<br>
    </blockquote>
    <br>
    I already gave you a solution that does not require a static size
    buffer.<br>
    <br>
    <blockquote type="cite">        while ( fgets ( line, sizeof line,
      file ) != NULL ){ /* read a line */<br>
                  string copy = string(line);<br>
                  //java tolerates spaces arround = char, remove them
      for matching<br>
    </blockquote>
    <br>
    s/arround/around/<br>
    <br>
    <blockquote type="cite">            remove_all_spaces(copy);<br>
                  //printf(line);<br>
                  //printf(copy.c_str());<br>
                  if (starts_with(copy,nwproperty)) {<br>
                      fclose (file);<br>
                      //provide non-spaced value, triming is  done in
      get_property_value<br>
    </blockquote>
    <br>
    s/triming/trimming/<br>
    <br>
    <blockquote type="cite">                get_property_value(copy,
      dest);<br>
                      return true;<br>
                      }<br>
                  }<br>
      <br>
              }else{<br>
                  perror(fileName.c_str()); /* why didn't the file open?
      */<br>
              }<br>
          return false;<br>
          }<br>
      <br>
      <br>
      //this is reimplmentation of itw-settings operations<br>
      //first check in user's settings, if found, return<br>
      //then check in global file (see the magic of
      find_system_config_file)<br>
      bool  read_deploy_property_value(string property, string&amp;
      dest){<br>
          //is it in user's file?<br>
          string keeper;<br>
          bool a = find_property(user_properties_file(), property,
      keeper);<br>
          if (a) {<br>
              dest=keeper;<br>
              return true;<br>
          }<br>
          //is it in global file?<br>
          bool b = find_system_config_file(keeper);<br>
          if (b) {<br>
              return find_property(keeper, property, dest);<br>
          }<br>
          return false;<br>
      }<br>
      <br>
      //This is different from common get property, as it is avoiding to
      search in *java*<br>
      //properties files<br>
      bool  find_custom_jre(string&amp; dest){<br>
          string key("deployment.jre.dir");<br>
    </blockquote>
    <br>
    It is fine to just use a const char* here (and have find_property
    take a const char*). std::string is mainly a benefit for mutable
    strings. It's correct, at least.<br>
    <br>
    <blockquote type="cite">    string keeper;<br>
          if(access(user_properties_file().c_str(), F_OK ) != -1 ) {<br>
              bool a = find_property(user_properties_file(),key,
      keeper);<br>
    </blockquote>
    <br>
    Nice variable name :-) ... you can embed this right in the if
    statement if you don't have a good name for it.<br>
    <br>
    <blockquote type="cite">        if (a) {<br>
                  dest = keeper;<br>
    </blockquote>
    <br>
    Pass 'dest' instead of 'keeper' to find_property and this will not
    be required.<br>
    <br>
    <blockquote type="cite">            return true;<br>
              }<br>
          }<br>
          if(access(main_properties_file().c_str(), F_OK ) != -1 ) {<br>
              return find_property(main_properties_file(),key, keeper);<br>
    </blockquote>
    <br>
    Bug: this does not modify 'dest'. Pass 'dest' instead of 'keeper'.<br>
    <br>
    <blockquote type="cite">    }<br>
      return false;<br>
      }<br>
      <br>
      <br>
      <br>
      int main(void){<br>
              printf("user's settings file\n");<br>
          cout &lt;&lt; user_properties_file();<br>
          printf("\nmain settings <a class="moz-txt-link-freetext" href="file:\n">file:\n</a>");<br>
          cout &lt;&lt; (main_properties_file());<br>
          printf("\njava settings file \n");<br>
          cout &lt;&lt; (getDefaultJavaPropertiesFile());<br>
          printf("\nsystem config file\n");<br>
          string a1;<br>
          find_system_config_file(a1);<br>
          cout &lt;&lt;  a1;<br>
          printf("\ncustom jre\n");<br>
          string a2;<br>
          find_custom_jre(a2);<br>
          cout &lt;&lt; a2;<br>
          printf("\nsome custom property\n");<br>
          string a3;<br>
          read_deploy_property_value("deployment.security.level", a3);<br>
          cout &lt;&lt; a3;<br>
          printf("\n");<br>
        return 0;<br>
      }</blockquote>
    <br>
    Not too bad :-)<br>
    <br>
    Keep at it! Just friendly review from intern etc etc<br>
    <br>
    Happy hacking,<br>
    -Adam<br>
  </body>
</html>