<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 03/19/2013 09:41 AM, Jiri Vanek
      wrote:<br>
    </div>
    <blockquote cite="mid:51486B08.10005@redhat.com" type="cite">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?
      <br>
      <br>
      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.
      <br>
    </blockquote>
    <br>
    Doesn't look half bad this time :-) Feel free to propose an
    integrated version. <br>
    Note that it seems the naming scheme for the CPP side is *.cc files
    using camelcase &amp; starting capital, so name it
    ParseProperties.cc, and give a header called ParseProperties.h.<br>
    (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 ?)<br>
    <br>
    re unit tests:<br>
    Examples can be found in tests/cpp-unit-tests.<br>
    <br>
    Steps:<br>
    1. Create a file called ParsePropertiesTest.cc <br>
    2. Include ParseProperties.h<br>
    3. Use the SUITE and TEST macros. Here's a suggested layout
    (Disclaimer: I *did not* compile this):<br>
    <br>
    #include &lt;cstdio&gt;<br>
    #include "ParseProperties.h"<br>
    <br>
    /* Creates a temporary file with the specified contents */<br>
    static std::string temporary_file(const std::string&amp; contents) {<br>
       std::string path = <b>tmpnam(NULL);</b> /* POSIX function, fine
    for test suite */<br>
       fstream file(path.c_str(), fstream::in); <br>
       file &lt;&lt; contents;<br>
       return path;<br>
    }<br>
    <br>
    SUITE(ParseProperties) { <br>
    <br>
        TEST(read_deploy_property_value) {<br>
            std::string value;<br>
            bool found = read_deploy_property_value("test", value);<br>
            CHECK(found);<br>
            CHECK(value == "foobar");<br>
        }<br>
    }<br>
    <br>
    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).<br>
    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.<br>
    <br>
    <blockquote cite="mid:51486B08.10005@redhat.com" type="cite">
      <br>
      Thanx for patient and detailed review!
      <br>
      <br>
      J.
      <br>
      <br>
      [..snip..]<br>
    </blockquote>
    <br>
    re new version:<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>
    </blockquote>
    <br>
    #include &lt;stdio.h&gt; -&gt; #include &lt;cstdio&gt;<br>
    #include &lt;stdlib.h&gt; -&gt; #include &lt;cstdlib&gt;<br>
    <br>
    I suppose it is easier if I just tell you what to do :-) Not a huge
    point anyway.<br>
    <br>
    <blockquote type="cite">#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>
      #include&lt;fstream&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>
    </blockquote>
    <br>
    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)<br>
    <br>
    <blockquote type="cite"><br>
      <br>
      // trim from start<br>
      static 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 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>
      // trim from both ends<br>
      static string &amp;trim(string&amp; s) {<br>
              return ltrim(rtrim(s));<br>
      }<br>
    </blockquote>
    <br>
    Ok you don't need to use my version but IMO you must:<br>
    1. Make this more readable. It is a simple operation, I would rather
    not use the rather complex templated algorithm &amp; functional
    library in C++<br>
    2. Either return 'string' and take 'string' (not 'string&amp;') OR
    take 'string&amp;' but do not return it. Returning a string makes it
    seem like the original string is unchanged.<br>
    <br>
    <blockquote type="cite"><br>
      <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[i] == '\t') {<br>
                  str.erase(i,1);<br>
              }<br>
          }<br>
      }<br>
      <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>
      <br>
      static bool file_exists(string filename)<br>
      {<br>
          ifstream infile(filename.c_str());<br>
          return infile.good();<br>
      }<br>
    </blockquote>
    <br>
    This is good, thanks.<br>
    <br>
    <blockquote type="cite"><br>
      <br>
      string  user_properties_file(){<br>
          int myuid = getuid();<br>
          struct passwd *mypasswd = getpwuid(myuid);<br>
          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 default_java_properties_file(){<br>
          return  ICEDTEA_WEB_JRE "/lib/deployment.properties";<br>
      }<br>
      <br>
      <br>
      //this is reimplemntation as icedtea-web settings do this (Also
      name of function is the same):<br>
    </blockquote>
    <br>
    s/ reimplemntation / reimplementation /<br>
    May be better worded as eg 'this is the same search done by
    icedtea-web settings'<br>
    <br>
    <blockquote type="cite">//try  the main file in
      /etc/.java/deployment<br>
    </blockquote>
    <blockquote type="cite">//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>
    </blockquote>
    <br>
    s/tryes/tries/<br>
    <br>
    <blockquote type="cite">//  if its  deploy file exists, then return<br>
      //not dound otherwise<br>
    </blockquote>
    <br>
    s/dound/found/<br>
    [Nit] Could you put the whole thing in a /**/ quote ?<br>
    <br>
    <blockquote type="cite">bool find_system_config_file(string&amp;
      dest){<br>
          if(file_exists(main_properties_file())) {<br>
              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(file_exists(jdest) ) {<br>
                      dest = jdest;<br>
                      return true;<br>
                  } <br>
              } else {<br>
                  if(file_exists(default_java_properties_file())) {<br>
                  dest = default_java_properties_file();<br>
                  return true;<br>
                  } <br>
              }    <br>
          }<br>
      return false; //nothing of above found<br>
      }<br>
      <br>
      //returns false if not found, true otherwise<br>
    </blockquote>
    <br>
    [nit] "Returns whether property was found, if found stores result in
    'dest'" is more descriptive.<br>
    <br>
    <blockquote type="cite">static bool  find_property(string filename,
      string property, string&amp; dest){<br>
          string nwproperty(property);<br>
          trim(nwproperty);<br>
          nwproperty=nwproperty+"=";<br>
    </blockquote>
    <br>
    [nit] It'd be clearer if this was something like string
    property_matcher = trim(property) + "="; (assuming a trim that
    returns a copy))<br>
    <br>
    <blockquote type="cite">    ifstream input( filename.c_str() );<br>
          for( string line; getline( input, line ); ){ /* read a line */<br>
              string copy = string(line);<br>
    </blockquote>
    <br>
    string(line) is redundant.  <br>
    <br>
    <blockquote type="cite">        //java tolerates spaces around =
      char, remove them for matching<br>
              remove_all_spaces(copy);<br>
    </blockquote>
    <blockquote type="cite">        //printf(line.c_str());<br>
              //printf(copy.c_str());<br>
    </blockquote>
    <br>
    Remember to drop these.<br>
    <br>
    <blockquote type="cite">        if (starts_with(copy,nwproperty)) {<br>
    </blockquote>
    <blockquote type="cite">            //provide non-spaced value,
      trimming is  done in get_property_value<br>
                  get_property_value(line, dest);<br>
                  return true;<br>
                  }<br>
              }<br>
      <br>
          return false;<br>
          }<br>
      <br>
      <br>
      //this is reimplmentation of itw-settings operations<br>
    </blockquote>
    <br>
    s/ reimplemntation / reimplementation /<br>
    May be better worded as eg 'this is the same search done by
    icedtea-web settings'<br>
    <br>
    <blockquote type="cite">//first check in user's settings, if found,
      return<br>
      //then check in global file (see the magic of
      find_system_config_file)<br>
    </blockquote>
    <br>
    [nit] I'd prefer a /**/ quote<br>
    <br>
    <blockquote type="cite">bool  read_deploy_property_value(string
      property, string&amp; dest){<br>
          //is it in user's file?<br>
          bool a = find_property(user_properties_file(), property,
      dest);<br>
          if (a) {<br>
              return true;<br>
          }<br>
          //is it in global file?<br>
          string futurefile;<br>
          bool b = find_system_config_file(futurefile);<br>
          if (b) {<br>
              return find_property(futurefile, 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>
          if(file_exists(user_properties_file())) {<br>
              bool a = find_property(user_properties_file(),key, dest);<br>
    </blockquote>
    <br>
    [nit] I still do not like this 'a' variable. (it makes some sense in
    read_deploy_property_value, but not here)<br>
    <br>
    <blockquote type="cite">        if (a) {<br>
                  return true;<br>
              }<br>
          }<br>
          if(file_exists(main_properties_file())) {<br>
              return find_property(main_properties_file(),key, dest);<br>
          }<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; (default_java_properties_file());<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>
    Good work!<br>
    -Adam<br>
  </body>
</html>