You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2008/05/16 17:27:11 UTC
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
arfrever@tigris.org writes:
> Author: arfrever
> Date: Fri May 16 10:09:13 2008
> New Revision: 31233
>
> Log:
> On the 'kwallet' branch:
>
> * subversion/libsvn_subr/cmdline.c
> (get_auth_simple_provider): Fix doc string and properly handle errors.
>
> Found by: danielsh
>
> Modified:
> branches/kwallet/subversion/libsvn_subr/cmdline.c
>
> Modified: branches/kwallet/subversion/libsvn_subr/cmdline.c
> URL: http://svn.collab.net/viewvc/svn/branches/kwallet/subversion/libsvn_subr/cmdline.c?pathrev=31233&r1=31232&r2=31233
> ==============================================================================
> --- branches/kwallet/subversion/libsvn_subr/cmdline.c Fri May 16 09:44:59 2008 (r31232)
> +++ branches/kwallet/subversion/libsvn_subr/cmdline.c Fri May 16 10:09:13 2008 (r31233)
> @@ -354,7 +354,7 @@ svn_cmdline_handle_exit_error(svn_error_
> return EXIT_FAILURE;
> }
>
> -/* Return dynamically loaded authentication simple provider. */
> +/* Dynamically load authentication simple provider. */
> static svn_boolean_t
> get_auth_simple_provider(svn_auth_provider_object_t **provider,
> const char *provider_name,
> @@ -372,7 +372,7 @@ get_auth_simple_provider(svn_auth_provid
> funcname = apr_psprintf(pool,
> "svn_auth_get_%s_simple_provider",
> provider_name);
> - svn_dso_load(&dso, libname);
> + svn_error_clear(svn_dso_load(&dso, libname));
> if (dso)
> {
> if (! apr_dso_sym(&provider_symbol, dso, funcname))
If svn_dso_load returns an error you cannot assume dso has a value you
can check. Further, simply ignoring all errors makes it hard for
users to debug problems with dynamic loading.
There ought to be some sort of version check similar to that in ra/fs
to prevent an incompatible library being used.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-05-16 22:33:37 Stefan Sperling napisał(a):
> On Fri, May 16, 2008 at 10:35:34PM +0300, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Fri, 16 May 2008 at 22:31 +0300:
> > > Philip Martin wrote on Fri, 16 May 2008 at 19:47 +0100:
> > > > "Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
> > > > > 2008-05-16 20:10 Philip Martin <ph...@codematters.co.uk> napisał(a):
> > > > >> Can the function return something otherthan SVN_NO_ERROR?
> > > > >
> >
> > Also, svn_dso_load is only declared and defined #if APR_HAS_DSO; and
> > /branches/kwallet/subversion/libsvn_subr/cmdline.c doesn't check for that
> > before calling it.
>
> Shouldn't the entire kwallet support depend upon APR_HAS_DSO then?
Fixed in r31260.
--
Arfrever Frehtes Taifersar Arahesis
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Stefan Sperling wrote on Fri, 16 May 2008 at 22:33 +0200:
> On Fri, May 16, 2008 at 10:35:34PM +0300, Daniel Shahaf wrote:
> > Also, svn_dso_load is only declared and defined #if APR_HAS_DSO; and
> > /branches/kwallet/subversion/libsvn_subr/cmdline.c doesn't check for that
> > before calling it.
>
> Shouldn't the entire kwallet support depend upon APR_HAS_DSO then?
>
Does defined(SVN_HAVE_KWALLET) == TRUE imply APR_HAS_DSO == TRUE?
I don't see any explicit DSO checks in the kwallet branch (I grepped
r31241).
> Geez, this is getting so complicated...
> And I thought the plaintext stuff was hard :/
>
> Stefan
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 16, 2008 at 10:35:34PM +0300, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Fri, 16 May 2008 at 22:31 +0300:
> > Philip Martin wrote on Fri, 16 May 2008 at 19:47 +0100:
> > > "Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
> > > > 2008-05-16 20:10 Philip Martin <ph...@codematters.co.uk> napisał(a):
> > > >> Can the function return something otherthan SVN_NO_ERROR?
> > > >
>
> Also, svn_dso_load is only declared and defined #if APR_HAS_DSO; and
> /branches/kwallet/subversion/libsvn_subr/cmdline.c doesn't check for that
> before calling it.
Shouldn't the entire kwallet support depend upon APR_HAS_DSO then?
Geez, this is getting so complicated...
And I thought the plaintext stuff was hard :/
Stefan
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Daniel Shahaf wrote on Fri, 16 May 2008 at 22:31 +0300:
> Philip Martin wrote on Fri, 16 May 2008 at 19:47 +0100:
> > "Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
> > > 2008-05-16 20:10 Philip Martin <ph...@codematters.co.uk> napisał(a):
> > >> Can the function return something otherthan SVN_NO_ERROR?
> > >
Also, svn_dso_load is only declared and defined #if APR_HAS_DSO; and
/branches/kwallet/subversion/libsvn_subr/cmdline.c doesn't check for that
before calling it.
>
> Philip, the answer is "Yes".
>
> > > See subversion/libsvn_subr/dso.c.
> >
> > I'm still having difficulty understanding what you mean, could you
> > explain rather than simply providing hints. Are you relying on
> > undocumented behaviour? If you want to rely on the function never
> > returning an error then you should change the documentation at the
> > very least, and probably the function prototype as well.
> >
>
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Philip Martin wrote on Fri, 16 May 2008 at 19:47 +0100:
> "Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
> > 2008-05-16 20:10 Philip Martin <ph...@codematters.co.uk> napisał(a):
> >> Can the function return something otherthan SVN_NO_ERROR?
> >
Philip, the answer is "Yes".
> > See subversion/libsvn_subr/dso.c.
>
> I'm still having difficulty understanding what you mean, could you
> explain rather than simply providing hints. Are you relying on
> undocumented behaviour? If you want to rely on the function never
> returning an error then you should change the documentation at the
> very least, and probably the function prototype as well.
>
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
"Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
> 2008-05-16 20:10 Philip Martin <ph...@codematters.co.uk> napisał(a):
>> "Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
>>
>>>>> - svn_dso_load(&dso, libname);
>>>>> + svn_error_clear(svn_dso_load(&dso, libname));
>>>>> if (dso)
>>>>> {
>>>>> if (! apr_dso_sym(&provider_symbol, dso, funcname))
>>>>
>>>> If svn_dso_load returns an error you cannot assume dso has a value you
>>>> can check. Further, simply ignoring all errors makes it hard for
>>>> users to debug problems with dynamic loading.
>>>
>>> /**
>>> * Attempt to load @a libname, returning it in @a dso.
>>> *
>>> * If @a libname cannot be loaded set @a dso to NULL and return
>>> * @c SVN_NO_ERROR.
>>> *
>>> * @note Due to pool lifetime issues DSOs are all loaded into a global
>>> * pool, so you must be certain that there is a bounded number of
>>> * them that will ever be loaded by the system, otherwise you will
>>> * leak memory.
>>> *
>>> * @since New in 1.4.0.
>>> */
>>> svn_error_t *svn_dso_load(apr_dso_handle_t **dso, const char *libname);
>>
>> Was your mail incomplete, I'm not sure what point you are trying to
>> make?
>
> It was complete. I just quoted the part of subversion/include/svn_dso.h.
>
>> Can the function return something otherthan SVN_NO_ERROR?
>
> See subversion/libsvn_subr/dso.c.
I'm still having difficulty understanding what you mean, could you
explain rather than simply providing hints. Are you relying on
undocumented behaviour? If you want to rely on the function never
returning an error then you should change the documentation at the
very least, and probably the function prototype as well.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-05-16 20:10 Philip Martin <ph...@codematters.co.uk> napisał(a):
> "Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
>
>>>> - svn_dso_load(&dso, libname);
>>>> + svn_error_clear(svn_dso_load(&dso, libname));
>>>> if (dso)
>>>> {
>>>> if (! apr_dso_sym(&provider_symbol, dso, funcname))
>>>
>>> If svn_dso_load returns an error you cannot assume dso has a value you
>>> can check. Further, simply ignoring all errors makes it hard for
>>> users to debug problems with dynamic loading.
>>
>> /**
>> * Attempt to load @a libname, returning it in @a dso.
>> *
>> * If @a libname cannot be loaded set @a dso to NULL and return
>> * @c SVN_NO_ERROR.
>> *
>> * @note Due to pool lifetime issues DSOs are all loaded into a global
>> * pool, so you must be certain that there is a bounded number of
>> * them that will ever be loaded by the system, otherwise you will
>> * leak memory.
>> *
>> * @since New in 1.4.0.
>> */
>> svn_error_t *svn_dso_load(apr_dso_handle_t **dso, const char *libname);
>
> Was your mail incomplete, I'm not sure what point you are trying to
> make?
It was complete. I just quoted the part of subversion/include/svn_dso.h.
> Can the function return something otherthan SVN_NO_ERROR?
See subversion/libsvn_subr/dso.c.
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
"Arfrever Frehtes Taifersar Arahesis" <ar...@gmail.com> writes:
>>> - svn_dso_load(&dso, libname);
>>> + svn_error_clear(svn_dso_load(&dso, libname));
>>> if (dso)
>>> {
>>> if (! apr_dso_sym(&provider_symbol, dso, funcname))
>>
>> If svn_dso_load returns an error you cannot assume dso has a value you
>> can check. Further, simply ignoring all errors makes it hard for
>> users to debug problems with dynamic loading.
>
> /**
> * Attempt to load @a libname, returning it in @a dso.
> *
> * If @a libname cannot be loaded set @a dso to NULL and return
> * @c SVN_NO_ERROR.
> *
> * @note Due to pool lifetime issues DSOs are all loaded into a global
> * pool, so you must be certain that there is a bounded number of
> * them that will ever be loaded by the system, otherwise you will
> * leak memory.
> *
> * @since New in 1.4.0.
> */
> svn_error_t *svn_dso_load(apr_dso_handle_t **dso, const char *libname);
Was your mail incomplete, I'm not sure what point you are trying to
make?
If the function returns SVN_NO_ERROR then you can check dso. Can the
function return something otherthan SVN_NO_ERROR? What's the point of
returning an svn_error_t if it is always SVN_NO_ERROR? If it does
return something other than SVN_NO_ERROR then you cannot check dso.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-05-16 Philip Martin <ph...@codematters.co.uk>:
> arfrever@tigris.org writes:
>
>> Author: arfrever
>> Date: Fri May 16 10:09:13 2008
>> New Revision: 31233
>>
>> Log:
>> On the 'kwallet' branch:
>>
>> * subversion/libsvn_subr/cmdline.c
>> (get_auth_simple_provider): Fix doc string and properly handle errors.
>>
>> Found by: danielsh
>>
>> Modified:
>> branches/kwallet/subversion/libsvn_subr/cmdline.c
>>
>> Modified: branches/kwallet/subversion/libsvn_subr/cmdline.c
>> URL: http://svn.collab.net/viewvc/svn/branches/kwallet/subversion/libsvn_subr/cmdline.c?pathrev=31233&r1=31232&r2=31233
>> ==============================================================================
>> --- branches/kwallet/subversion/libsvn_subr/cmdline.c Fri May 16 09:44:59 2008 (r31232)
>> +++ branches/kwallet/subversion/libsvn_subr/cmdline.c Fri May 16 10:09:13 2008 (r31233)
>> @@ -354,7 +354,7 @@ svn_cmdline_handle_exit_error(svn_error_
>> return EXIT_FAILURE;
>> }
>>
>> -/* Return dynamically loaded authentication simple provider. */
>> +/* Dynamically load authentication simple provider. */
>> static svn_boolean_t
>> get_auth_simple_provider(svn_auth_provider_object_t **provider,
>> const char *provider_name,
>> @@ -372,7 +372,7 @@ get_auth_simple_provider(svn_auth_provid
>> funcname = apr_psprintf(pool,
>> "svn_auth_get_%s_simple_provider",
>> provider_name);
>> - svn_dso_load(&dso, libname);
>> + svn_error_clear(svn_dso_load(&dso, libname));
>> if (dso)
>> {
>> if (! apr_dso_sym(&provider_symbol, dso, funcname))
>
> If svn_dso_load returns an error you cannot assume dso has a value you
> can check. Further, simply ignoring all errors makes it hard for
> users to debug problems with dynamic loading.
/**
* Attempt to load @a libname, returning it in @a dso.
*
* If @a libname cannot be loaded set @a dso to NULL and return
* @c SVN_NO_ERROR.
*
* @note Due to pool lifetime issues DSOs are all loaded into a global
* pool, so you must be certain that there is a bounded number of
* them that will ever be loaded by the system, otherwise you will
* leak memory.
*
* @since New in 1.4.0.
*/
svn_error_t *svn_dso_load(apr_dso_handle_t **dso, const char *libname);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31233 - branches/kwallet/subversion/libsvn_subr
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-05-16 19:27:11 Philip Martin napisał(a):
> There ought to be some sort of version check similar to that in ra/fs
> to prevent an incompatible library being used.
Fixed in r33305.
--
Arfrever Frehtes Taifersar Arahesis