Re: [PATCH] Fix stack smashing bugs

David Anderson wrote:
> Stefan Küng wrote:
> > [[[
> > Add progress notification for ra_svn. This is for issue #901.
> >
> > Patch by: Stefan Küng <tortoisesvn@gmail.com>
> >
> > * subversion/libsvn_ra_svn/client.c (ra_svn_open): Initialize the
> > progress callback function for svn connections.
>
> Incorrect formatting for the log message here. Not sure whether this
> is my client messing up, so just in case.

I'll fix that.

> > Index: subversion/libsvn_ra_svn/client.c
> > ===================================================================
> > --- subversion/libsvn_ra_svn/client.c (Revision 15964)
> > +++ subversion/libsvn_ra_svn/client.c (Arbeitskopie)
> > @@ -644,6 +644,11 @@
> > conn = svn_ra_svn_create_conn(sock, NULL, NULL, pool);
> > }
> >
> > + conn->progress_func = callbacks->progress_func;
> > + conn->progress_baton = callbacks->progress_baton;
> > + conn->progress_progress = 0;
> > + conn->progress_total = -1;
>
> Why is progress_total initialized to -1 ? As you're not touching it
> again in the patch, I'm supposing this is because you're indicating
> the total is "not yet supported". Could you expand on this (in a
> mail, plus perhaps a small comment in the code), and perhaps offer a
> potential solution to make it supported if you have an idea ?

Here, the structure is only initialized. Even progress_progress is set
to 0, and that's used later on.
The reason progress_total isn't used later on is because I haven't found
a way to set that value to a meaningful value. I've tried to set it
before some certain function calls, but in every function I've tried it
wasn't really useful.

How do you suggest should I add such a comment to the code?

> > Index: subversion/libsvn_ra_svn/marshal.c
> > ===================================================================
> > --- subversion/libsvn_ra_svn/marshal.c (Revision 15964)
> > +++ subversion/libsvn_ra_svn/marshal.c (Arbeitskopie)
> > @@ -156,6 +156,12 @@
> > status = apr_socket_send(conn->sock, data, &count);
> > else
> > status = apr_file_write(conn->out_file, data, &count);
> > + conn->progress_progress += count;
> > + if (conn->progress_func)
> > + {
> > + conn->progress_func(conn->progress_progress,
> conn->progress_total,
> > + pool, conn->progress_baton);
> > + }
>
> > @@ -247,6 +253,12 @@
> > status = apr_socket_recv(conn->sock, data, len);
> > else
> > status = apr_file_read(conn->in_file, data, len);
> > + conn->progress_progress += *len;
> > + if (conn->progress_func)
> > + {
> > + conn->progress_func(conn->progress_progress,
> conn->progress_total,
> > + pool, conn->progress_baton);
> > + }
>
> How does the progress notification callback know when the progress is
> write progress and when it is read progress? You seem to be calling
> the exact same callback each time round. What happens if reads and
> writes are interlaced? How does the callback figure out what to tell
> the user?

In the case of DAV which uses the neon callback for the progress
information, it also isn't known if the data transferred was sent or
received. And I'm not sure if it's possible to do that with the neon
callback.
I think for GUI clients (which will be most likely the clients using
this) they just can show the user something like "transferred 12345bytes
at 1.2kBytes/s". Even though that's not even close to what issue #901
covers, it is enough to show the user at least that *something* is going
on. If we don't have this, then every client appears to 'hang' or do
nothing when e.g. a very big file is imported.

> > + SVN_ERR(readbuf_input(conn, data, &count, pool));
>
> A global comment on the addition of the extra pool parameter.
> Actually, more of a question to know which is preferred: should this
> extra parameter be added given that is only concerns progress
> notification, or should the pool be passed in the svn_ra_svn_conn_t as
> a *progress_pool parameter?

That's another possibility to pass the pool. Haven't even thought about
this.
But I don't know which is better here - maybe someone more familliar
with pools can tell which one to prefer?

> > Index: subversion/libsvn_ra_svn/ra_svn.h
> > ===================================================================
> > --- subversion/libsvn_ra_svn/ra_svn.h (Revision 15964)
> > +++ subversion/libsvn_ra_svn/ra_svn.h (Arbeitskopie)
> > @@ -29,6 +29,7 @@
> > #include <apr_file_io.h>
> > #include <apr_thread_proc.h>
> > #include <svn_ra_svn.h>
>
> On a completely unrelated note, shouldn't this be #include
> "svn_ra_svn.h" ?

I was wondering about that too. And if I should include the svn_ra.h
with '<>' too. But then, the '<>' is usually only used for includes of
libraries in the PATH, not project includes.

> > @@ -53,6 +54,10 @@
> > ra_svn_block_handler_t block_handler;
> > void *block_baton;
> > apr_hash_t *capabilities;
> > + svn_ra_progress_notify_func_t progress_func;
> > + void *progress_baton;
> > + apr_off_t progress_progress;
> > + apr_off_t progress_total;
>
> Not indispensable given the surrounding code, but a few comments here,
> along with a line skip between *capabilities and progress_func would
> be nice. Even though the rest of the structure is somewhat lacking on
> the comment side, no need to make it worse by adding *more*
> undocumented fields :-).

Something like:

   /* the callback function for progress information and the baton */
   svn_ra_progress_notify_func_t progress_func;
   void *progress_baton;
   /* values used to collect progress information and pass them to the
      progress notification callbacks */
   apr_off_t progress_progress;
   apr_off_t progress_total;
   apr_pool_t *pool;

Stefan

-- 
        ___
   oo  // \\      "De Chelonian Mobile"
  (_,\/ \_/ \     TortoiseSVN
    \ \_/_\_/>    The coolest Interface to (Sub)Version Control
    /_/   \_\     http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org