Re: [bug-myserver] Preliminary to gopher support :)

[Giuseppe Scrivano]

Hello!


Domenico Chierico <address@hidden> writes:

> hi all!!
>
> this is perhaps useless .. but it's not my own idea :)

Thank you to have worked on it.  Useless or not, Gopher is a nice
addition :-)

> +class GopherEngine;
> +
> +typedef void (GopherEngine::* MyMethodPtr)(string, string,
> GopherMenu&);
> +
> +class GopherEngine
> +{
> +public:
> +  GopherEngine ();
> +  GopherContent &incoming (GopherRequest, Vhost *tmp);  
> +
> +protected:
> +  void dirManagement (string fname, string path, GopherMenu &tmp);
> +  void fileManagement (string fname, string path ,GopherMenu &tmp);
> +
> +  void infoFile (string fname, string path, GopherMenu &tmp);
> +  void textFile (string fname, string path, GopherMenu &tmp);
> +  void csoFile (string fname, string path, GopherMenu &tmp);
> +  void binhexFile (string fname, string path, GopherMenu &tmp);
> +  void dosbinFile (string fname, string path, GopherMenu &tmp);
> +  void UUencodeFile (string fname, string path, GopherMenu &tmp);
> +  void telnetFile (string fname, string path, GopherMenu &tmp);
> +  void binFile (string fname, string path, GopherMenu &tmp);
> +  void imageFile (string fname, string path, GopherMenu &tmp);
> +  void gifFile (string fname, string path, GopherMenu &tmp);

What about using string references in these functions?  I see these
values are only read and never modified.

For example:
  void gifFile (const string &fname, const string &path, GopherMenu &tmp);


> +  virtual char* registerName(char* out, int len)
> +  {
> +    return Gopher::registerNameImpl(out, len);
> +  }

Please leave a free empty space between the function name and the first
'('.  Example:

virtual char* registerName (char* out, int len)
{
  return Gopher::registerNameImpl (out, len);
}


> diff --git a/myserver/include/protocol/gopher/gopher_content.h

Leave an empty space between the function name and '(' in this file too,
I omitted its content.


> diff --git a/myserver/src/protocol/Makefile.am
> \ No newline at end of file

Please leave a new line at the end of Makefile.am.


> diff --git a/myserver/src/protocol/gopher/engine.cpp
> b/myserver/src/protocol/gopher/engine.cpp
> +++ b/myserver/src/protocol/gopher/engine.cpp

> +  path.append ("/");
> +  path.append (req.getRequest ());
> +  if(! fd.findfirst (path.c_str ()))
> +    {
> +      Gu = new GopherMenu;
> +      if(FilesUtility::getPathRecursionLevel (path)>0)
> +     {
> +       do {
> +         if(fd.name[0]!='.')
> +           {
> +             cout << fd.name << " : " << fd.attrib << endl;
> +             if (fd.attrib & FILE_ATTRIBUTE_DIRECTORY)
> +               {
> +                 dirManagement (fd.name, req.getRequest (), *Gu);
> +               }
> +             else
> +               {
> +                 fileManagement (fd.name, req.getRequest (),  *Gu);          
>     
> +               }
> +           }
> +       }while(!fd.findnext ());
> +     }
> +      else
> +     {
> +       Gu->addIntro(string("Invalid requested Path\n"), hostname, port);
> +     }

Use the gettext macro _ around text messages:

  Gu->addIntro(string(_("Invalid requested Path\n")), hostname, port);


> +      fd.findclose ();
> +    }
> +  else
> +    {
> +      Gud = (GopherContent*)new GopherFileContent (path);
> +      return *Gud;
> +    }
> +  return *Gu;
> +}

Please correct the indentation, if you are using Emacs use this
configuration before indent it automatically by tab:

(setq c-basic-offset 2)
(setq-default indent-tabs-mode nil) 


Also, please don't use {} for a single line statement:

                if (fd.attrib & FILE_ATTRIBUTE_DIRECTORY)
      dirManagement (fd.name, req.getRequest (), *Gu);
                else
      fileManagement (fd.name, req.getRequest (),  *Gu);                    


> +  string f_path = path;
> +  if (f_path != ""){
> +    f_path.append ("/");
> +  }

No need for {} here too.  In any case the { will be on a new line:

if (something)
  {
    foo ();
    bar ();
  }


> +  f_path.append (fname);
> +  string abs_name = abs_path;
> +  if (abs_name != ""){
> +    abs_name.append ("/");
> +  }

Here too.


> +  abs_name.append(f_path);
> +
> +  FilesUtility::getFileExt (ext,fname);
> +  if (handlers.containsKey (ext.c_str ()))
> +    {
> +      (this->*handlers.get(ext.c_str ())) (fname, path, tmp);
> +    }

And here.


> +  /*const char *handler = td->securityToken.getData ("mime.handler",
> +    MYSERVER_VHOST_CONF | MYSERVER_SERVER_CONF, NULL);*/

Please don't leave commented out code.  If it is not needed then it must
be dropped.


> +  if (f_path != ""){
> +    f_path.append ("/");
> +  }

Not needed {...}.


> +/*!
> + * Destructor for the http class.
> + */

s/http/gopher/


> +Gopher::~Gopher ()
> +{
> +  /*  clean (); */
> +}

this line should be dropped, it is commented-out code.



> +/*!
> + * Main Clean method usefull in different env
> + */

s/usefull/useful/


> +
> +/* void Gopher::clean(){} */

Drop it.



> +int Gopher::loadProtocolStatic ()
> +{
> +  /* boh */
> +}

If it is possible, better remove this comment :-)


> +  Reply (pConnection,Gu);
> +}
> +
> +void Gopher::Reply(ConnectionPtr pConnection,
> +                GopherContent &data)
> +{
> +  data.toProtocol(pConnection->socket);
> +  pConnection->socket->close();
> +}

Please don't capitalize function names: 

  void Gopher::reply (ConnectionPtr pConnection.....



> +++ b/myserver/src/protocol/gopher/gopher_content.cpp

> +{
> +  size_t size = freeText.size();
> +  char myline[size];
> +  vector<GopherItem> tmp;
> +  istream *res;
> +  istringstream str(freeText);
> +  while(!str.getline(myline,size).eof())

Please leave an empty space between the function name and '(' -- this is
for all the file.  Leave a space after the comma:

  while (!str.getline (myline, size).eof ())


> +GopherItem::~GopherItem()
> +{
> +  /** clean everithig **/

I would remove this comment, or fix the typo.


> +  s->send(res.c_str(), strlen(res.c_str()),0);

Here too you can make it faster accessing directly res.length () instead
of strlen (res.c_str ()).  A white space before '0'.

> +  init('0',d,loc,h,p);
> +  init('1',"","","","");
> +  init('1',d,loc,h,p);
> +  init('2',"","","","");
> +  init('2',d,loc,h,p);
> +  init('3',"","","","");
> +  init('3',d,loc,h,p);
> +  init('4',"","","","");
> +  init('4',d,loc,h,p);
> +  init('5',"","","","");
> +  init('5',d,loc,h,p);
> +  init('6',"","","","");
> +  init('6',d,loc,h,p);
> +  init('7',"","","","");
> +  init('7',d,loc,h,p);
> +  init('8',"","","","");
> +  init('8',d,loc,h,p);
> +  init('9',"","","","");
> +  init('9',d,loc,h,p);
> +  init(0,"","","","");
> +  init(0,d,loc,h,p);
> +  init('g',"","","","");
> +  init('g',d,loc,h,p);
> +  init('g',"","","","");
> +  init('I',d,loc,h,p);

All these calls don't leave proper white spaces after any comma.


> +void GopherData::toProtocol(Socket *s)
> +{
> +  string res = buff;
> +  res.append("\r\n.\r\n");
> +  s->send(res.c_str(), strlen(res.c_str()),0);

Use res.length ().


> @@ -360,6 +361,7 @@ void Server::loadPlugins ()
>  
>    Protocol *protocolsSet[] = {new HttpProtocol (),
>                                new HttpsProtocol (),
> +                           new GopherProtocol (),
>                                new FtpProtocol (),
>                                new ControlProtocol (),
>                                NULL};

Please respect the indendation and remove any tab, we indent the code
using white spaces.  Be sure that there are not tabs in the code.

Thanks again for your work!
Giuseppe