You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by vijay <vi...@collab.net> on 2011/03/01 07:29:28 UTC
[PATCH] Compiling subversion trunk with httpd trunk code fails
Hi,
I tried to compile subversion trunk with httpd trunk code. make fails
with the attached error.
Then I got that there is a macro APLOG_MARK which expands differently in
httpd-2.2.x and httpd-2.3-dev.
From httpd-2.2.x,
#define APLOG_MARK __FILE__,__LINE__
From httpd-2.3-dev,
#define APLOG_MARK __FILE__,__LINE__,APLOG_MODULE_INDEX
The method
"subversion/mod_authz_svn/mod_authz_svn.c:log_access_verdict()" is being
called with the argument APLOG_MARK in few places. As it is getting
expanded to three arguments in httpd-2.3-dev, but the orginal
implementation of log_access_verdict accepts only two arguments(file and
line), make fails with "error: too many arguments to function
‘log_access_verdict’".
Attaching the patch that works for httpd-trunk as well as older versions.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by Kamesh Jayachandran <ka...@collab.net>.
Thanks Vijay.
Committed your in r1076234.
With regards
Kamesh Jayachandran
On 03/02/2011 03:18 PM, vijay wrote:
> Attached the patch that uses macro based solution. APLOG_USE_MODULE is
> used only in case of HTTPD 2.3.
>
> Thanks & Regards,
> Vijayaguru
>
> On Wednesday 02 March 2011 12:20 AM, Stefan Sperling wrote:
>> On Tue, Mar 01, 2011 at 11:08:22PM +0530, Kamesh Jayachandran wrote:
>>>> On the whole I preferred the macro solution.
>>> Me to prefer the orignal macro based solution.
>> OK. Let's go for that, then.
>
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by vijay <vi...@collab.net>.
Attached the patch that uses macro based solution. APLOG_USE_MODULE is
used only in case of HTTPD 2.3.
Thanks & Regards,
Vijayaguru
On Wednesday 02 March 2011 12:20 AM, Stefan Sperling wrote:
> On Tue, Mar 01, 2011 at 11:08:22PM +0530, Kamesh Jayachandran wrote:
>>> On the whole I preferred the macro solution.
>> Me to prefer the orignal macro based solution.
> OK. Let's go for that, then.
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 01, 2011 at 11:08:22PM +0530, Kamesh Jayachandran wrote:
> >On the whole I preferred the macro solution.
>
> Me to prefer the orignal macro based solution.
OK. Let's go for that, then.
RE: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by Kamesh Jayachandran <ka...@collab.net>.
>No! If you are going to use separate functions then write:
>static void log_access_verdict(...)
>{
It still needs a signature change to log_access_verdict.
>#if AP_MODULE_MAGIC_AT_LEAST(...)
> log_access_verdict_httpd_v23(...)
>#else
> log_access_verdict_httpd_v22(...)
>#endif
>}
>so that the callers don't have to change.
>On the whole I preferred the macro solution.
Me to prefer the orignal macro based solution.
With regards
Kamesh Jayachandran
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by Philip Martin <ph...@wandisco.com>.
vijay <vi...@collab.net> writes:
> * This function is used as a provider to allow mod_dav_svn to bypass the
> @@ -580,7 +624,11 @@
> if (!conf->anonymous
> || (! (conf->access_file || conf->repo_relative_access_file)))
> {
> - log_access_verdict(APLOG_MARK, r, 0, repos_path, NULL);
> +#if AP_MODULE_MAGIC_AT_LEAST(20100606,0)
> + log_access_verdict_httpd_v23(APLOG_MARK, r, 0, repos_path, NULL);
> +#else
> + log_access_verdict_httpd_v22(APLOG_MARK, r, 0, repos_path, NULL);
> +#endif
No! If you are going to use separate functions then write:
static void log_access_verdict(...)
{
#if AP_MODULE_MAGIC_AT_LEAST(...)
log_access_verdict_httpd_v23(...)
#else
log_access_verdict_httpd_v22(...)
#endif
}
so that the callers don't have to change.
On the whole I preferred the macro solution.
--
Philip
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by vijay <vi...@collab.net>.
Herewith attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
On Tuesday 01 March 2011 06:30 PM, Stefan Sperling wrote:
> On Tue, Mar 01, 2011 at 11:59:28AM +0530, vijay wrote:
>> [[[
>> Update log_access_verdict to make it work with httpd trunk as well as older
>> versions. The function is being called with APLOG_MACRO in few places.
>> The macro APLOG_MARK expands to 2 arguments till httpd-2.2.x but 3
>> arguments in httpd-2.3-dev, which causes failure while compiling with
>> httpd-2.3-dev. So we need to handle both the cases.
>>
>> * subversion/mod_authz_svn/mod_authz_svn.c
>> (log_access_verdict): Modify the signature.
> Your log message should probably include this link:
> http://httpd.apache.org/docs/trunk/developer/new_api_2_4.html#upgrading_logging
>
>> Patch by: Vijayaguru G<vijay{_AT_}collab.net>
>> Suggested by: Kameshj
>> ]]]
>>
>>
>> Index: subversion/mod_authz_svn/mod_authz_svn.c
>> ===================================================================
>> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 1075316)
>> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
>> @@ -32,6 +32,7 @@
>> #include<http_log.h>
>> #include<ap_config.h>
>> #include<ap_provider.h>
>> +#include<ap_release.h>
>> #include<apr_uri.h>
>> #include<apr_lib.h>
>> #include<mod_dav.h>
>> @@ -519,12 +520,20 @@
>> return OK;
>> }
>>
>> +#if AP_SERVER_MAJORVERSION_NUMBER == 2&& AP_SERVER_MINORVERSION_NUMBER == 3
>> +#define LOG_ARGS_SIGNATURE const char *file, int line, int module_index
>> +#define LOG_ARGS_CASCADE file, line, module_index
>> +#else
>> +#define LOG_ARGS_SIGNATURE const char *file, int line
>> +#define LOG_ARGS_CASCADE file, line
>> +#endif
> I don't really like a macro that expands to function parameters.
> Can we do something like this instead?
> This will duplicate some code, but the function is small and this way
> it's a bit easier to understand what is happening at a glance.
>
> #if AP_MODULE_MAGIC_AT_LEAST(2,3)
> static void
> log_access_verdict_httpd_v23(const char *file, int line,
> const request_rec *r, int allowed,
> const char *repos_path,
> const char *dest_repos_path)
> {
> /* code for v23 */
> }
> #else
> static void
> log_access_verdict2_httpd_v22(const char *file, int line, int module_index,
> const request_rec *r, int allowed,
> const char *repos_path, const char *dest_repos_path)
> {
> /* code for v22 */
> }
> #endif
>
> static void
> log_access_verdict(const char *file, int line, int module_index,
> const request_rec *r, int allowed,
> const char *repos_path, const char *dest_repos_path)
> {
> #if AP_MODULE_MAGIC_AT_LEAST(2,3)
> /* call log_access_verdict_httpd_v23 */
> #endif
> /* call log_access_verdict_httpd_v22 */
> #endif
> }
>
>> /* Log a message indicating the access control decision made about a
>> - * request. FILE and LINE should be supplied via the APLOG_MARK macro.
>> - * ALLOWED is boolean. REPOS_PATH and DEST_REPOS_PATH are information
>> + * request. LOG_ARGS_SIGNATURE should be supplied via the APLOG_MARK macro
>> + * with respect to Apache version. ALLOWED is boolean.
>> + * REPOS_PATH and DEST_REPOS_PATH are information
>> * about the request. DEST_REPOS_PATH may be NULL. */
>> static void
>> -log_access_verdict(const char *file, int line,
>> +log_access_verdict(LOG_ARGS_SIGNATURE,
>> const request_rec *r, int allowed,
>> const char *repos_path, const char *dest_repos_path)
>> {
>> @@ -534,22 +543,22 @@
>> if (r->user)
>> {
>> if (dest_repos_path)
>> - ap_log_rerror(file, line, level, 0, r,
>> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
>> "Access %s: '%s' %s %s %s", verdict, r->user,
>> r->method, repos_path, dest_repos_path);
>> else
>> - ap_log_rerror(file, line, level, 0, r,
>> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
>> "Access %s: '%s' %s %s", verdict, r->user,
>> r->method, repos_path);
>> }
>> else
>> {
>> if (dest_repos_path)
>> - ap_log_rerror(file, line, level, 0, r,
>> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
>> "Access %s: - %s %s %s", verdict,
>> r->method, repos_path, dest_repos_path);
>> else
>> - ap_log_rerror(file, line, level, 0, r,
>> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
>> "Access %s: - %s %s", verdict,
>> r->method, repos_path);
>> }
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 01, 2011 at 11:59:28AM +0530, vijay wrote:
> [[[
> Update log_access_verdict to make it work with httpd trunk as well as older
> versions. The function is being called with APLOG_MACRO in few places.
> The macro APLOG_MARK expands to 2 arguments till httpd-2.2.x but 3
> arguments in httpd-2.3-dev, which causes failure while compiling with
> httpd-2.3-dev. So we need to handle both the cases.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (log_access_verdict): Modify the signature.
Your log message should probably include this link:
http://httpd.apache.org/docs/trunk/developer/new_api_2_4.html#upgrading_logging
> Patch by: Vijayaguru G <vijay{_AT_}collab.net>
> Suggested by: Kameshj
> ]]]
>
>
> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 1075316)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -32,6 +32,7 @@
> #include <http_log.h>
> #include <ap_config.h>
> #include <ap_provider.h>
> +#include <ap_release.h>
> #include <apr_uri.h>
> #include <apr_lib.h>
> #include <mod_dav.h>
> @@ -519,12 +520,20 @@
> return OK;
> }
>
> +#if AP_SERVER_MAJORVERSION_NUMBER == 2 && AP_SERVER_MINORVERSION_NUMBER == 3
> +#define LOG_ARGS_SIGNATURE const char *file, int line, int module_index
> +#define LOG_ARGS_CASCADE file, line, module_index
> +#else
> +#define LOG_ARGS_SIGNATURE const char *file, int line
> +#define LOG_ARGS_CASCADE file, line
> +#endif
I don't really like a macro that expands to function parameters.
Can we do something like this instead?
This will duplicate some code, but the function is small and this way
it's a bit easier to understand what is happening at a glance.
#if AP_MODULE_MAGIC_AT_LEAST(2,3)
static void
log_access_verdict_httpd_v23(const char *file, int line,
const request_rec *r, int allowed,
const char *repos_path,
const char *dest_repos_path)
{
/* code for v23 */
}
#else
static void
log_access_verdict2_httpd_v22(const char *file, int line, int module_index,
const request_rec *r, int allowed,
const char *repos_path, const char *dest_repos_path)
{
/* code for v22 */
}
#endif
static void
log_access_verdict(const char *file, int line, int module_index,
const request_rec *r, int allowed,
const char *repos_path, const char *dest_repos_path)
{
#if AP_MODULE_MAGIC_AT_LEAST(2,3)
/* call log_access_verdict_httpd_v23 */
#endif
/* call log_access_verdict_httpd_v22 */
#endif
}
> /* Log a message indicating the access control decision made about a
> - * request. FILE and LINE should be supplied via the APLOG_MARK macro.
> - * ALLOWED is boolean. REPOS_PATH and DEST_REPOS_PATH are information
> + * request. LOG_ARGS_SIGNATURE should be supplied via the APLOG_MARK macro
> + * with respect to Apache version. ALLOWED is boolean.
> + * REPOS_PATH and DEST_REPOS_PATH are information
> * about the request. DEST_REPOS_PATH may be NULL. */
> static void
> -log_access_verdict(const char *file, int line,
> +log_access_verdict(LOG_ARGS_SIGNATURE,
> const request_rec *r, int allowed,
> const char *repos_path, const char *dest_repos_path)
> {
> @@ -534,22 +543,22 @@
> if (r->user)
> {
> if (dest_repos_path)
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: '%s' %s %s %s", verdict, r->user,
> r->method, repos_path, dest_repos_path);
> else
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: '%s' %s %s", verdict, r->user,
> r->method, repos_path);
> }
> else
> {
> if (dest_repos_path)
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: - %s %s %s", verdict,
> r->method, repos_path, dest_repos_path);
> else
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: - %s %s", verdict,
> r->method, repos_path);
> }
Re: [PATCH] Compiling subversion trunk with httpd trunk code fails
Posted by Philip Martin <ph...@wandisco.com>.
vijay <vi...@collab.net> writes:
> Index: subversion/mod_authz_svn/mod_authz_svn.c
> ===================================================================
> --- subversion/mod_authz_svn/mod_authz_svn.c (revision 1075316)
> +++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
> @@ -32,6 +32,7 @@
> #include <http_log.h>
> #include <ap_config.h>
> #include <ap_provider.h>
> +#include <ap_release.h>
> #include <apr_uri.h>
> #include <apr_lib.h>
> #include <mod_dav.h>
> @@ -519,12 +520,20 @@
> return OK;
> }
>
> +#if AP_SERVER_MAJORVERSION_NUMBER == 2 && AP_SERVER_MINORVERSION_NUMBER == 3
I think this should be ap_mmm.h and AP_MODULE_MAGIC_AT_LEAST.
Perhaps we should be using AP_DECLARE_MODULE or AP_USE_MODULE somewhere
in mod_authz_svn?
> +#define LOG_ARGS_SIGNATURE const char *file, int line, int module_index
> +#define LOG_ARGS_CASCADE file, line, module_index
> +#else
> +#define LOG_ARGS_SIGNATURE const char *file, int line
> +#define LOG_ARGS_CASCADE file, line
> +#endif
> /* Log a message indicating the access control decision made about a
> - * request. FILE and LINE should be supplied via the APLOG_MARK macro.
> - * ALLOWED is boolean. REPOS_PATH and DEST_REPOS_PATH are information
> + * request. LOG_ARGS_SIGNATURE should be supplied via the APLOG_MARK macro
> + * with respect to Apache version. ALLOWED is boolean.
> + * REPOS_PATH and DEST_REPOS_PATH are information
> * about the request. DEST_REPOS_PATH may be NULL. */
> static void
> -log_access_verdict(const char *file, int line,
> +log_access_verdict(LOG_ARGS_SIGNATURE,
> const request_rec *r, int allowed,
> const char *repos_path, const char *dest_repos_path)
> {
> @@ -534,22 +543,22 @@
> if (r->user)
> {
> if (dest_repos_path)
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: '%s' %s %s %s", verdict, r->user,
> r->method, repos_path, dest_repos_path);
> else
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: '%s' %s %s", verdict, r->user,
> r->method, repos_path);
> }
> else
> {
> if (dest_repos_path)
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: - %s %s %s", verdict,
> r->method, repos_path, dest_repos_path);
> else
> - ap_log_rerror(file, line, level, 0, r,
> + ap_log_rerror(LOG_ARGS_CASCADE, level, 0, r,
> "Access %s: - %s %s", verdict,
> r->method, repos_path);
> }
--
Philip