Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

2024-04-22 Thread Ruediger Pluem



On 4/22/24 4:07 PM, Graham Leggett via dev wrote:
> On 22 Apr 2024, at 14:05, Ruediger Pluem  wrote:
> 
 Now the caller needs to allocate memory even if it is not interested in 
 the error or if there is no error at all.
 Wouldn't it be better to add an apu_err_t **err instead (which can be 
 NULL) and in case of an error allocate an
 apu_err_t from pool and fill it and return it in **err (provided it was 
 not NULL).
>>>
>>> I originally used this pattern and found it overkill - what I do now is 
>>> allocate a app_err_t on the stack and immediately
>>> abandon it.
>>
>> Did you commit this?
> 
> Updated in http://svn.apache.org/viewvc?rev=1917271=rev

But I don't see where you allocate the structure on the stack on the caller 
side for apr_crypto and apr_dbm and only copy it to a
pool based alloc in case of an error and that we pass NULL in case of apr_dbd 
where we don't process it.
Currently we allocate memory for an apu_err_t struct that we don't need in most 
cases as it only matters in case of an error
which I hope is not the typical code path.

Regards

Rüdiger



Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

2024-04-22 Thread Graham Leggett via dev
On 22 Apr 2024, at 14:05, Ruediger Pluem  wrote:

>>> Now the caller needs to allocate memory even if it is not interested in the 
>>> error or if there is no error at all.
>>> Wouldn't it be better to add an apu_err_t **err instead (which can be NULL) 
>>> and in case of an error allocate an
>>> apu_err_t from pool and fill it and return it in **err (provided it was not 
>>> NULL).
>> 
>> I originally used this pattern and found it overkill - what I do now is 
>> allocate a app_err_t on the stack and immediately abandon it.
> 
> Did you commit this?

Updated in http://svn.apache.org/viewvc?rev=1917271=rev

Regards,
Graham
--



Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

2024-04-22 Thread Ruediger Pluem



On 4/17/24 9:30 AM, Graham Leggett via dev wrote:
> On 17 Apr 2024, at 08:07, Ruediger Pluem  wrote:
> 
>>> Modified: apr/apr/trunk/util-misc/apu_dso.c
>>> URL: 
>>> http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apu_dso.c?rev=1917050=1917049=1917050=diff
>>> ==
>>> --- apr/apr/trunk/util-misc/apu_dso.c (original)
>>> +++ apr/apr/trunk/util-misc/apu_dso.c Tue Apr 16 21:33:58 2024
>>> @@ -131,7 +131,8 @@ apr_status_t apu_dso_load(apr_dso_handle
>>>   apr_dso_handle_sym_t *dsoptr,
>>>   const char *module,
>>>   const char *modsym,
>>> -  apr_pool_t *pool)
>>> +  apr_pool_t *pool,
>>> +  apu_err_t *err)
>>
>> Now the caller needs to allocate memory even if it is not interested in the 
>> error or if there is no error at all.
>> Wouldn't it be better to add an apu_err_t **err instead (which can be NULL) 
>> and in case of an error allocate an
>> apu_err_t from pool and fill it and return it in **err (provided it was not 
>> NULL).
> 
> I originally used this pattern and found it overkill - what I do now is 
> allocate a app_err_t on the stack and immediately abandon it.

Did you commit this?

Regards

Rüdiger



Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

2024-04-17 Thread Graham Leggett via dev
On 17 Apr 2024, at 08:07, Ruediger Pluem  wrote:

>> Modified: apr/apr/trunk/util-misc/apu_dso.c
>> URL: 
>> http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apu_dso.c?rev=1917050=1917049=1917050=diff
>> ==
>> --- apr/apr/trunk/util-misc/apu_dso.c (original)
>> +++ apr/apr/trunk/util-misc/apu_dso.c Tue Apr 16 21:33:58 2024
>> @@ -131,7 +131,8 @@ apr_status_t apu_dso_load(apr_dso_handle
>>   apr_dso_handle_sym_t *dsoptr,
>>   const char *module,
>>   const char *modsym,
>> -  apr_pool_t *pool)
>> +  apr_pool_t *pool,
>> +  apu_err_t *err)
> 
> Now the caller needs to allocate memory even if it is not interested in the 
> error or if there is no error at all.
> Wouldn't it be better to add an apu_err_t **err instead (which can be NULL) 
> and in case of an error allocate an
> apu_err_t from pool and fill it and return it in **err (provided it was not 
> NULL).

I originally used this pattern and found it overkill - what I do now is 
allocate a app_err_t on the stack and immediately abandon it.

In this particular case we need to always handle this error. I've had cases 
where I've brought httpd up onto a debugger to find out why I was getting 
incomprehensible failures elsewhere with no explanation, only to discover that 
it was as simple as the RPM containing the individual APR driver wasn't 
installed.

What I can do is if(err) { ... } for the allocations.

Regards,
Graham
--



Re: svn commit: r1917050 - in /apr/apr/trunk: CHANGES crypto/apr_crypto.c dbd/apr_dbd.c dbm/apr_dbm.c include/private/apu_internal.h util-misc/apu_dso.c

2024-04-17 Thread Ruediger Pluem



On 4/16/24 11:33 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Tue Apr 16 21:33:58 2024
> New Revision: 1917050
> 
> URL: http://svn.apache.org/viewvc?rev=1917050=rev
> Log:
> Pass details of module loading errors back to the caller through the
> apu_err_t structure.
> 
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/crypto/apr_crypto.c
> apr/apr/trunk/dbd/apr_dbd.c
> apr/apr/trunk/dbm/apr_dbm.c
> apr/apr/trunk/include/private/apu_internal.h
> apr/apr/trunk/util-misc/apu_dso.c
> 

> Modified: apr/apr/trunk/util-misc/apu_dso.c
> URL: 
> http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apu_dso.c?rev=1917050=1917049=1917050=diff
> ==
> --- apr/apr/trunk/util-misc/apu_dso.c (original)
> +++ apr/apr/trunk/util-misc/apu_dso.c Tue Apr 16 21:33:58 2024
> @@ -131,7 +131,8 @@ apr_status_t apu_dso_load(apr_dso_handle
>apr_dso_handle_sym_t *dsoptr,
>const char *module,
>const char *modsym,
> -  apr_pool_t *pool)
> +  apr_pool_t *pool,
> +  apu_err_t *err)

Now the caller needs to allocate memory even if it is not interested in the 
error or if there is no error at all.
Wouldn't it be better to add an apu_err_t **err instead (which can be NULL) and 
in case of an error allocate an
apu_err_t from pool and fill it and return it in **err (provided it was not 
NULL).


>  {
>  apr_dso_handle_t *dlhandle = NULL;
>  char *pathlist;
> @@ -186,6 +187,7 @@ apr_status_t apu_dso_load(apr_dso_handle
>  apr_cpystrn(eos, module, sizeof(path) - (eos - path));
>  
>  rv = apr_dso_load(, path, global);
> +
>  if (dlhandleptr) {
>  *dlhandleptr = dlhandle;
>  }
> @@ -205,20 +207,27 @@ apr_status_t apu_dso_load(apr_dso_handle
>  apr_cpystrn(eos, module, sizeof(path) - (eos - path));
>  
>  rv = apr_dso_load(, path, global);
> +
>  if (dlhandleptr) {
>  *dlhandleptr = dlhandle;
>  }
> +
>  if (rv == APR_SUCCESS) { /* APR_EDSOOPEN */
>  break;
>  }
>  }
>  }
>  
> -if (rv != APR_SUCCESS) /* APR_ESYMNOTFOUND */
> +if (rv != APR_SUCCESS) { /* APR_ESYMNOTFOUND */
> +err->msg = apr_pstrdup(pool, apr_dso_error(dlhandle, NULL, 0));
> +err->reason = apr_pstrdup(pool, module);
>  return rv;
> +}
>  
>  rv = apr_dso_sym(dsoptr, dlhandle, modsym);
>  if (rv != APR_SUCCESS) { /* APR_ESYMNOTFOUND */
> +err->msg = apr_pstrdup(pool, apr_dso_error(dlhandle, NULL, 0));
> +err->reason = apr_pstrdup(pool, module);
>  apr_dso_unload(dlhandle);
>  }
>  else {
> @@ -231,5 +240,5 @@ apr_status_t apu_dso_load(apr_dso_handle
>  return rv;
>  }
>  
> -#endif /* APR_DSO_BUILD */
> +#endif /* APR_HAVE_MODULAR_DSO */

Regards

Rüdiger