VK Sameer <sameer@collab.net> writes:
> > > * subversion/libsvn_ra_svn/serve.c:
> > > (lock_many): New function
> > > (unlock_many): New function
> >
> > I haven't looked at the code yet, but I can see in the log message
> > that you added two static functions to libsvn_ra_svn/serve.c (they
> > must be static, because they have no "svn_..." prefixes). Since
> > they're static, they can only be called from within the same file.
> > Yet there are no other changes listed for that file, so no calls to
> > these new functions were added. Either:
> >
> > a) The log message is incomplete, or
> > b) These functions really are unused :-).
> >
> > I'm guessing it's (a). On to the code...
>
> These get called by being present in main_commands[]. OK, will add more
> verbiage.
Yup. Since it's just a change to the object named 'main_commands',
you can add
(main_commands): Add entries for lock_many and unlock_many.
to the entry for serve.c
> > (not "@pool", by the way). But they're not; errors are always
> > allocated in subpools of the global pool. @a pool is only used here
> > for temporary work.
> >
> > Also, the function returns exactly one error, yet the doc string
> > claims it "generates errors". How are these multiple errors returned?
> > Are they an error chain dangling off one returned error? I guess so,
> > based on the code later.
> >
> > Here's an attempt at a restatement:
> >
> > /** Return an error chain based on @a params (which contains a
> > * command response indicating failure). The error chain will be
> > * in the same order as the errors indicated in @a params.
> > *
> > * Use @pool for temporary allocation.
> > */
> >
> > Is that right?
>
> I'm not sure. The function seems to be building an error chain using the
> incoming @a pool. My ignorance of pool usage is probably woefully
> obvious, but since the errors allocated using pool will be used in the
> caller, is it really for temporary allocation?
Are you sure that's what's going on? Search for "pool" in that code,
and watch carefully for the difference between "pool" and "err->pool".
I think there's only one use of "pool", and it's not involved in
creating or chaining the error objects.
> Yes, but save_paths is used later in the function. The sequence
> ra_svn_lock and ra_svn_unlock follows is:
>
> loop #1 to build batch svn command
> send batch command
> loop #2 to handle batch command response
>
> Some information built up in loop #1 using subpool is used later in loop
> #2. Hence the non-idiomatic use of subpool. Does it make more sense to
> use pool throughout?
Within a loop, you should use a persistent pool to allocate
information that needs to persist, and a (non-persistent) loop pool
for everything else. You don't need to choose one pool to use
throughout the loop. Instead, use the appropriate pool in each
instance, even if that means using different pools for different
things inside the same loop.
> Also, my pool usage may be very simplistic since I avoided using the
> incoming "pool" parameter once "subpool" was created. Using "subpool"
> alone throughout the function seemed to reduce the effort in figuring
> out when a pool needs to be destroyed at function return points. (Not
> that I have been consistent in the destruction).
I know what you mean. But the thing is, readers of Subversion code
take pool choices as clues to object lifetimes. By seeing which pool
was used, we have hints as to whether something is meant to persist.
For example, look at svn_repos_trace_node_locations(), see the way
lastpool and currpool are used. That's about correctness, of course,
but it's also a huge clue to the reader about the algorithm there.
The code would be much harder to understand if a single subpool had
been used for everything.
> Yes, I definitely need to understand pool usage better.
Are there specific questions that the relevant section of HACKING
leaves unanswered? I'm a little too familiar with it now to be able
to see weaknesses in the explanation there, unfortunately, but I'd be
happy to help clarify anything. Plus it wouldn't hurt to expand the
explanation in HACKING, if it's not sufficient. Let us know.
> Aha, so that explains why I don't have to worry about subpool leaking
> memory through an SVN_ERR. There's still the issue of save_paths,
> however.
save_paths should be allocated in pool, not subpool. The subpool
should still be used for the call to svn_ra_svn_write_tuple() *inside*
the for-loop, but not for the call above the for-loop. The subpool
should be cleared at the start of each loop, and destroyed when the
loop is done. Outside loops, you should just use pool.
I haven't talked about the second for-loop, but from the above you can
probably guess what should go on there (or if it's not clear, please
don't hesitate to ask -- I don't mean this as a test, I just don't
want to bombard you with information if things are already clear).
> > Hey, I have an idea! Since the protocol words are "unlock_many" and
> > "unlock", why don't you make those be the names of the corresponding
> > internal functions? In other words, "ra_svn_unlock_many()", etc.
> > Same goes for the locking functions.
>
> Keeping the old names was based on Ringstrom's and Fitz's comments on
> IRC about not revving the interface. The current functions already have
> the required multi-path interface, so there would be no difference in
> the parameters of ra_svn_lock and ra_svn_lock_many. A new function name
> would also need propagation to libsvn_ra code as well.
Oops, I didn't understand this. These are internal static functions.
They are not part of any published interface, and therefore there are
no API concerns -- no "revving" would be happening here. You can call
them anything you want; you might as well call them something that
matches their protocol commands in an intuitive way.
When you say "the current functions", which ones exactly are you
referring to?
> > This looks like you added support for handling servers that don't know
> > about "get-locks", but you *removed* support for handling servers that
> > don't know about "get-lock". That's bad, and not just because it
> > makes the above comment inapplicable to the code! :-)
>
> Umm, it was actually a copy-paste fix :) since the message is in
> ra_svn_get_locks(), not ra_svn_get_lock(). But as it's totally unrelated
> to this patch, I'll take it out.
Ah, thanks. Yeah, should be a separate patch, then.
> > > +{
> > > + apr_array_header_t *locks;
> > > + apr_array_header_t *lock_cmds;
> > > + server_baton_t *b = baton;
> > > + const char *comment;
> > > + svn_boolean_t force;
> > > + int i;
> > > + struct lock_cmd {
> > > + const char *path;
> > > + const char *full_path;
> > > + svn_revnum_t current_rev;
> > > + svn_lock_t *l;
> > > + };
> > > +
> > > + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?c)bl", &comment, &force,
> > > + &locks));
> > > +
> > > + lock_cmds =
> > > + apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
> > > +
> > > + /* Loop through the lock commands */
> > > + for (i = 0; i < locks->nelts; ++i)
> > > + {
> > > + struct lock_cmd *cmd = apr_pcalloc(pool, sizeof(struct lock_cmd));
> > > + svn_ra_svn_item_t *item =
> > > + &APR_ARRAY_IDX(locks, i, svn_ra_svn_item_t);
> > > +
> > > + if (item->kind != SVN_RA_SVN_LIST)
> > > + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> > > + "Lock commands should be list of lists\n");
> > > +
> > > + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?r)",
> > > + &cmd->path, &cmd->current_rev));
> > > +
> > > + cmd->full_path = svn_path_join(b->fs_path,
> > > + svn_path_canonicalize(cmd->path, pool),
> > > + pool);
> > > +
> > > + APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> > > + }
> > > +
> > > + SVN_ERR(must_have_write_access(conn, pool, b, TRUE));
> > > +
> > > + /* Loop through each path to be locked */
> > > + for (i = 0; i < lock_cmds->nelts; i++)
> > > + {
> > > + struct lock_cmd *cmd =
> > > + &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> > > +
> > > + SVN_CMD_ERR(
> > > + svn_repos_fs_lock(&cmd->l, b->repos, cmd->full_path,
> > > + NULL, comment, 0,
> > > + 0, /* No expiration time. */
> > > + cmd->current_rev,
> > > + force, pool));
> > > + }
> > > +
> > > + /* (success( (ccc(?c)c(?c) ... )) */
> > > + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(!", "success"));
> > > + for (i = 0; i < lock_cmds->nelts; i++)
> > > + {
> > > + struct lock_cmd *cmd =
> > > + &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> > > + SVN_ERR(write_lock(conn, pool, cmd->l));
> > > + }
> > > + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)"));
> > > +
> > > + return SVN_NO_ERROR;
> > > +}
> >
> > Again with the loop subpools. But you'll have to be careful, because
> > some of the data needs to persist beyond the lifetime of the loop in
> > which it was generated. Choose your pools carefully.
>
> Hmm, can't I just stick with subpool?
See far above about using the right pool for each occasion, mixing
different pools in the same loop, with super-fine granularity :-).
> > Gosh. I feel like there's a big code duplication going on, but I'm
> > not sure if lock_many and unlock_many can be cleanly factored with
> > each other. Might be worth trying; your call.
>
> Unfortunately, the params seem ever-so-slightly incompatible. But will
> take a look again.
Okay, I can see how that would be the case. Just thought I'd ask.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 31 19:12:01 2005