You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <rh...@sharpsvn.net> on 2009/05/12 15:42:01 UTC
RE: svn commit: r37709 - trunk/subversion/libsvn_subr
> -----Original Message-----
> From: Arfrever Frehtes Taifersar Arahesis [mailto:Arfrever.FTA@GMail.Com]
> Sent: dinsdag 12 mei 2009 17:31
> To: svn@subversion.tigris.org
> Subject: svn commit: r37709 - trunk/subversion/libsvn_subr
>
> Author: arfrever
> Date: Tue May 12 08:30:42 2009
> New Revision: 37709
>
> Log:
> When SVN_DEBUG is defined, print error messages containing details about
> failures of dynamic loading of libraries performed by apr_dso_load().
>
> * subversion/libsvn_subr/dso.c
> (svn_dso_load): Print message retrieved by apr_dso_error().
Why is this necessary?
If the error is useful, the full error message can be returned (as wrapped
error?) in the svn_error_t* returned by this function and if it is not, what
is the use of printing it in debug mode?
Bert
>
> Modified:
> trunk/subversion/libsvn_subr/dso.c
>
> Modified: trunk/subversion/libsvn_subr/dso.c
> URL:
>
http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/dso.c?pathrev=
37
> 709&r1=37708&r2=37709
>
============================================================================
==
> --- trunk/subversion/libsvn_subr/dso.c Tue May 12 08:29:24 2009
(r37708)
> +++ trunk/subversion/libsvn_subr/dso.c Tue May 12 08:30:42 2009
(r37709)
> @@ -98,6 +98,10 @@ svn_dso_load(apr_dso_handle_t **dso, con
> status = apr_dso_load(dso, fname, dso_pool);
> if (status)
> {
> +#ifdef SVN_DEBUG
> + char buf[1024];
> + fprintf(stderr, "%s\n", apr_dso_error(*dso, buf, 1024));
> +#endif
> *dso = NULL;
>
> /* It wasn't found, so set the special "we didn't find it"
value.
> */
>
> ------------------------------------------------------
>
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=221
59
> 43
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2216038
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Branko Cibej <br...@xbc.nu>.
Greg Stein wrote:
> On Wed, May 13, 2009 at 12:50, Branko Čibej <br...@xbc.nu> wrote:
>
>> Arfrever Frehtes Taifersar Arahesis wrote:
>>
>>> 2009-05-12 21:11 Greg Stein <gs...@gmail.com> napisał(a):
>>>
>>>
>>>> On Tue, May 12, 2009 at 20:20, Branko Cibej <br...@xbc.nu> wrote:
>>>>
>>>>
>>>>> Arfrever Frehtes Taifersar Arahesis wrote:
>>>>>
>>>>>
>>>>>>> Why is this necessary?
>>>>>>>
>>>>>>> If the error is useful, the full error message can be returned (as wrapped
>>>>>>> error?) in the svn_error_t* returned by this function
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> The doc string of svn_dso_load() contains:
>>>>>> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>>>>>>
>>>>>> The error message might be useful to find bugs such as the one introduced
>>>>>> in r35309+r35385+r36823.
>>>>>>
>>>>>>
>>>>>>
>>>>> Never mind if it might be useful; we do not print anything from our
>>>>> libraries. It's just Not Done and I don't care about broken libraries
>>>>> that do expect to be able to print anything.
>>>>>
>>>>>
>>>> To be fair, the print is within SVN_DEBUG which means "only svn devs"
>>>>
>>>> But yah: rev the API and improve the error return. Consumers can
>>>> always throw out the error.
>>>>
>>>>
>>> `svn`, `svnsync` etc. should probably print the error message
>>> retrieved from apr_dso_error(), but they must try to continue to work
>>> when dynamic loading of a library failed. For example, when dynamic
>>> loading of libsvn_ra_neon-1.so.0 fails during accessing a repository
>>> through http:// protocol, libsvn_ra_serf-1.so.0 might be available, so
>>> libsvn_ra-1.so.0 should try to dynamically load libsvn_ra_serf-1.so.0.
>>>
>>> svn_dso_load() is called only by Subversion libraries (libsvn_subr,
>>> libsvn_fs, libsvn_subr), not by `svn` and other programs, so I'm not
>>> sure how to implement sending of error message from a library function
>>> (which calls svn_dso_load()) to a program without returning error from
>>> this function.
>>>
>>>
>> Why do you want to make things complicated? You can safely assume that
>> *if* there's a chance that an alternate DSO can be used, the caller of
>> svn_dso_load should take care of that silently. I don't want to see "You
>> requested serf but we're using neon instead" printed every time I do
>> anything network related; the semantics should be the same, a user
>> doesn't give a damn which library is used as long as stuff works.
>>
>> If, on the other hand, we really do decide that such info is an
>> important user-facing interface, then by golly you shouldn't just print
>> it, you should have your caller of svn_dso_load accept a notification
>> callback parameter -- same as we do for all other progress and status
>> reporting. But that's a decision to be made on a case-by-case basis and
>> does not belong in svn_dso_load.
>>
>
> Yeah, a callback would be the answer. And you note, selected on a
> case-by-case basis. For this one: no way. Try to load one module, and
> hold onto the error while trying to load the alternate. If you can't
> load *either*, then return the composition of the two errors.
>
> No information is lost. Done.
>
I meant "case by case" as in, use a callback depending on what the
caller is doing. No arguments about compositing errors.
-- Brane
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2236604
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Greg Stein <gs...@gmail.com>.
On Wed, May 13, 2009 at 12:50, Branko Čibej <br...@xbc.nu> wrote:
> Arfrever Frehtes Taifersar Arahesis wrote:
>> 2009-05-12 21:11 Greg Stein <gs...@gmail.com> napisał(a):
>>
>>> On Tue, May 12, 2009 at 20:20, Branko Cibej <br...@xbc.nu> wrote:
>>>
>>>> Arfrever Frehtes Taifersar Arahesis wrote:
>>>>
>>>>>> Why is this necessary?
>>>>>>
>>>>>> If the error is useful, the full error message can be returned (as wrapped
>>>>>> error?) in the svn_error_t* returned by this function
>>>>>>
>>>>>>
>>>>> The doc string of svn_dso_load() contains:
>>>>> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>>>>>
>>>>> The error message might be useful to find bugs such as the one introduced
>>>>> in r35309+r35385+r36823.
>>>>>
>>>>>
>>>> Never mind if it might be useful; we do not print anything from our
>>>> libraries. It's just Not Done and I don't care about broken libraries
>>>> that do expect to be able to print anything.
>>>>
>>> To be fair, the print is within SVN_DEBUG which means "only svn devs"
>>>
>>> But yah: rev the API and improve the error return. Consumers can
>>> always throw out the error.
>>>
>>
>> `svn`, `svnsync` etc. should probably print the error message
>> retrieved from apr_dso_error(), but they must try to continue to work
>> when dynamic loading of a library failed. For example, when dynamic
>> loading of libsvn_ra_neon-1.so.0 fails during accessing a repository
>> through http:// protocol, libsvn_ra_serf-1.so.0 might be available, so
>> libsvn_ra-1.so.0 should try to dynamically load libsvn_ra_serf-1.so.0.
>>
>> svn_dso_load() is called only by Subversion libraries (libsvn_subr,
>> libsvn_fs, libsvn_subr), not by `svn` and other programs, so I'm not
>> sure how to implement sending of error message from a library function
>> (which calls svn_dso_load()) to a program without returning error from
>> this function.
>>
>
> Why do you want to make things complicated? You can safely assume that
> *if* there's a chance that an alternate DSO can be used, the caller of
> svn_dso_load should take care of that silently. I don't want to see "You
> requested serf but we're using neon instead" printed every time I do
> anything network related; the semantics should be the same, a user
> doesn't give a damn which library is used as long as stuff works.
>
> If, on the other hand, we really do decide that such info is an
> important user-facing interface, then by golly you shouldn't just print
> it, you should have your caller of svn_dso_load accept a notification
> callback parameter -- same as we do for all other progress and status
> reporting. But that's a decision to be made on a case-by-case basis and
> does not belong in svn_dso_load.
Yeah, a callback would be the answer. And you note, selected on a
case-by-case basis. For this one: no way. Try to load one module, and
hold onto the error while trying to load the alternate. If you can't
load *either*, then return the composition of the two errors.
No information is lost. Done.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2235583
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Branko Cibej <br...@xbc.nu>.
Arfrever Frehtes Taifersar Arahesis wrote:
> 2009-05-12 21:11 Greg Stein <gs...@gmail.com> napisał(a):
>
>> On Tue, May 12, 2009 at 20:20, Branko Cibej <br...@xbc.nu> wrote:
>>
>>> Arfrever Frehtes Taifersar Arahesis wrote:
>>>
>>>>> Why is this necessary?
>>>>>
>>>>> If the error is useful, the full error message can be returned (as wrapped
>>>>> error?) in the svn_error_t* returned by this function
>>>>>
>>>>>
>>>> The doc string of svn_dso_load() contains:
>>>> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>>>>
>>>> The error message might be useful to find bugs such as the one introduced
>>>> in r35309+r35385+r36823.
>>>>
>>>>
>>> Never mind if it might be useful; we do not print anything from our
>>> libraries. It's just Not Done and I don't care about broken libraries
>>> that do expect to be able to print anything.
>>>
>> To be fair, the print is within SVN_DEBUG which means "only svn devs"
>>
>> But yah: rev the API and improve the error return. Consumers can
>> always throw out the error.
>>
>
> `svn`, `svnsync` etc. should probably print the error message
> retrieved from apr_dso_error(), but they must try to continue to work
> when dynamic loading of a library failed. For example, when dynamic
> loading of libsvn_ra_neon-1.so.0 fails during accessing a repository
> through http:// protocol, libsvn_ra_serf-1.so.0 might be available, so
> libsvn_ra-1.so.0 should try to dynamically load libsvn_ra_serf-1.so.0.
>
> svn_dso_load() is called only by Subversion libraries (libsvn_subr,
> libsvn_fs, libsvn_subr), not by `svn` and other programs, so I'm not
> sure how to implement sending of error message from a library function
> (which calls svn_dso_load()) to a program without returning error from
> this function.
>
Why do you want to make things complicated? You can safely assume that
*if* there's a chance that an alternate DSO can be used, the caller of
svn_dso_load should take care of that silently. I don't want to see "You
requested serf but we're using neon instead" printed every time I do
anything network related; the semantics should be the same, a user
doesn't give a damn which library is used as long as stuff works.
If, on the other hand, we really do decide that such info is an
important user-facing interface, then by golly you shouldn't just print
it, you should have your caller of svn_dso_load accept a notification
callback parameter -- same as we do for all other progress and status
reporting. But that's a decision to be made on a case-by-case basis and
does not belong in svn_dso_load.
-- Brane
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2234920
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-05-12 21:11 Greg Stein <gs...@gmail.com> napisał(a):
> On Tue, May 12, 2009 at 20:20, Branko Cibej <br...@xbc.nu> wrote:
>> Arfrever Frehtes Taifersar Arahesis wrote:
>>>> Why is this necessary?
>>>>
>>>> If the error is useful, the full error message can be returned (as wrapped
>>>> error?) in the svn_error_t* returned by this function
>>>>
>>>
>>> The doc string of svn_dso_load() contains:
>>> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>>>
>>> The error message might be useful to find bugs such as the one introduced
>>> in r35309+r35385+r36823.
>>>
>>
>> Never mind if it might be useful; we do not print anything from our
>> libraries. It's just Not Done and I don't care about broken libraries
>> that do expect to be able to print anything.
>
> To be fair, the print is within SVN_DEBUG which means "only svn devs"
>
> But yah: rev the API and improve the error return. Consumers can
> always throw out the error.
`svn`, `svnsync` etc. should probably print the error message
retrieved from apr_dso_error(), but they must try to continue to work
when dynamic loading of a library failed. For example, when dynamic
loading of libsvn_ra_neon-1.so.0 fails during accessing a repository
through http:// protocol, libsvn_ra_serf-1.so.0 might be available, so
libsvn_ra-1.so.0 should try to dynamically load libsvn_ra_serf-1.so.0.
svn_dso_load() is called only by Subversion libraries (libsvn_subr,
libsvn_fs, libsvn_subr), not by `svn` and other programs, so I'm not
sure how to implement sending of error message from a library function
(which calls svn_dso_load()) to a program without returning error from
this function.
--
Arfrever Frehtes Taifersar Arahesis
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2234615
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Greg Stein <gs...@gmail.com>.
On Tue, May 12, 2009 at 20:20, Branko Cibej <br...@xbc.nu> wrote:
> Arfrever Frehtes Taifersar Arahesis wrote:
>>> Why is this necessary?
>>>
>>> If the error is useful, the full error message can be returned (as wrapped
>>> error?) in the svn_error_t* returned by this function
>>>
>>
>> The doc string of svn_dso_load() contains:
>> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>>
>> The error message might be useful to find bugs such as the one introduced
>> in r35309+r35385+r36823.
>>
>
> Never mind if it might be useful; we do not print anything from our
> libraries. It's just Not Done and I don't care about broken libraries
> that do expect to be able to print anything.
To be fair, the print is within SVN_DEBUG which means "only svn devs"
But yah: rev the API and improve the error return. Consumers can
always throw out the error.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2219332
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Branko Cibej <br...@xbc.nu>.
Arfrever Frehtes Taifersar Arahesis wrote:
>> Why is this necessary?
>>
>> If the error is useful, the full error message can be returned (as wrapped
>> error?) in the svn_error_t* returned by this function
>>
>
> The doc string of svn_dso_load() contains:
> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>
> The error message might be useful to find bugs such as the one introduced
> in r35309+r35385+r36823.
>
Never mind if it might be useful; we do not print anything from our
libraries. It's just Not Done and I don't care about broken libraries
that do expect to be able to print anything.
Greg's suggestion is the right one.
-- Brane
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2218596
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Greg Stein <gs...@gmail.com>.
On Tue, May 12, 2009 at 18:12, Arfrever Frehtes Taifersar Arahesis
<Ar...@gmail.com> wrote:
> 2009-05-12 17:42:01 Bert Huijben napisał(a):
>> > -----Original Message-----
>> > From: Arfrever Frehtes Taifersar Arahesis [mailto:Arfrever.FTA@GMail.Com]
>> > Sent: dinsdag 12 mei 2009 17:31
>> > To: svn@subversion.tigris.org
>> > Subject: svn commit: r37709 - trunk/subversion/libsvn_subr
>> >
>> > Author: arfrever
>> > Date: Tue May 12 08:30:42 2009
>> > New Revision: 37709
>> >
>> > Log:
>> > When SVN_DEBUG is defined, print error messages containing details about
>> > failures of dynamic loading of libraries performed by apr_dso_load().
>> >
>> > * subversion/libsvn_subr/dso.c
>> > (svn_dso_load): Print message retrieved by apr_dso_error().
>>
>> Why is this necessary?
>>
>> If the error is useful, the full error message can be returned (as wrapped
>> error?) in the svn_error_t* returned by this function
>
> The doc string of svn_dso_load() contains:
> "If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
>
> The error message might be useful to find bugs such as the one introduced
> in r35309+r35385+r36823.
How about rev'ing the API and returning a proper error?
(and the old API can continue to suck at error reporting... we don't
care since we'll update our calls)
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2216746
Re: svn commit: r37709 - trunk/subversion/libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <Ar...@GMail.Com>.
2009-05-12 17:42:01 Bert Huijben napisał(a):
> > -----Original Message-----
> > From: Arfrever Frehtes Taifersar Arahesis [mailto:Arfrever.FTA@GMail.Com]
> > Sent: dinsdag 12 mei 2009 17:31
> > To: svn@subversion.tigris.org
> > Subject: svn commit: r37709 - trunk/subversion/libsvn_subr
> >
> > Author: arfrever
> > Date: Tue May 12 08:30:42 2009
> > New Revision: 37709
> >
> > Log:
> > When SVN_DEBUG is defined, print error messages containing details about
> > failures of dynamic loading of libraries performed by apr_dso_load().
> >
> > * subversion/libsvn_subr/dso.c
> > (svn_dso_load): Print message retrieved by apr_dso_error().
>
> Why is this necessary?
>
> If the error is useful, the full error message can be returned (as wrapped
> error?) in the svn_error_t* returned by this function
The doc string of svn_dso_load() contains:
"If @a libname cannot be loaded set @a dso to NULL and return @c SVN_NO_ERROR."
The error message might be useful to find bugs such as the one introduced
in r35309+r35385+r36823.
--
Arfrever Frehtes Taifersar Arahesis