Re: [PATCH] fix up external acl type dumping
On Fri, Jun 15, 2012 at 4:23 AM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/14/2012 03:06 AM, Robert Collins wrote: +#define DUMP_EXT_ACL_TYPE_FMT(a, fmt, ...) \ + case _external_acl_format::EXT_ACL_##a: \ + storeAppendPrintf(sentry, fmt, ##__VA_ARGS__); \ + break I do not see Squid using __VA_ARGS__ currently. Are you sure it is portable, especially in the ## context? If you have not tested this outside Linux, are you OK with us pulling out this patch if it breaks the build? Yes, that would be fine. I'm mainly fixing this because I noticed it in passing: it has no concrete effect on me. If you are not sure, you can rework the macro to always use a single argument instead. The calls without a parameter in the format can add one and use an empty string (or you can have two macros). I do not like how this macro butchers the format code names. I could borderline agree with stripping the namespace, but stripping EXT_ACL_ prefix seems excessive. The prefix itself looks excessive (because the namespace already has external_acl in it!) but that is a different issue. I would prefer that the macro takes a real code name constant, without mutilating the name. There is an existing similar macro that does the same kind of mutilation, but there it is kind of justified because the name suffix is actually used. The true solution to this mess is to fix the names themselves, I guess. I think we should make external acls a class, no base class, and ditch the whole big case statement etc, and we can lose the macro at the same time. Thats a rather bigger change, and I wanted to fix the defect in the first instance. -Rob
Re: [PATCH] fix up external acl type dumping
tor 2012-06-14 klockan 10:23 -0600 skrev Alex Rousskov: +#define DUMP_EXT_ACL_TYPE_FMT(a, fmt, ...) \ +case _external_acl_format::EXT_ACL_##a: \ +storeAppendPrintf(sentry, fmt, ##__VA_ARGS__); \ +break I do not see Squid using __VA_ARGS__ currently. Are you sure it is portable, especially in the ## context? If you have not tested this outside Linux, are you OK with us pulling out this patch if it breaks the build? Checking.. seems all platform we care about should support it by now. http://en.wikipedia.org/wiki/Variadic_macro But there is some variance on how an empty list of variadic arguments is handled. Some require at least one argument to avoid syntax errors in resulting code. (the , before ##__VA_ARGS__ always emitted) Regards HEnrik
Re: [PATCH] fix up external acl type dumping
On 06/15/2012 06:16 AM, Henrik Nordström wrote: tor 2012-06-14 klockan 10:23 -0600 skrev Alex Rousskov: +#define DUMP_EXT_ACL_TYPE_FMT(a, fmt, ...) \ +case _external_acl_format::EXT_ACL_##a: \ +storeAppendPrintf(sentry, fmt, ##__VA_ARGS__); \ +break I do not see Squid using __VA_ARGS__ currently. Are you sure it is portable, especially in the ## context? If you have not tested this outside Linux, are you OK with us pulling out this patch if it breaks the build? Checking.. seems all platform we care about should support it by now. http://en.wikipedia.org/wiki/Variadic_macro But there is some variance on how an empty list of variadic arguments is handled. Some require at least one argument to avoid syntax errors in resulting code. (the , before ##__VA_ARGS__ always emitted) And since the patch does not always supply that argument, the probability that we will have to fix this after commit is high. IMHO, ##__VA_ARGS__ is not worth the trouble in this particular case. However, even if you disagree, please use at least one argument (empty string with a corresponding %s if needed to prevent compiler warnings?). Thank you, Alex.
Re: [PATCH] fix up external acl type dumping
On Sat, Jun 16, 2012 at 7:33 AM, Alex Rousskov rouss...@measurement-factory.com wrote: IMHO, ##__VA_ARGS__ is not worth the trouble in this particular case. However, even if you disagree, please use at least one argument (empty string with a corresponding %s if needed to prevent compiler warnings?). The ## is meant to compensate for that, according to the reading I did. I'm happy to back it out if it causes jenkins failures, but would like to at least try it as-is. -Rob
Re: [PATCH] fix up external acl type dumping
On 16/06/2012 7:41 a.m., Robert Collins wrote: On Sat, Jun 16, 2012 at 7:33 AM, Alex Rousskov rouss...@measurement-factory.com wrote: IMHO, ##__VA_ARGS__ is not worth the trouble in this particular case. However, even if you disagree, please use at least one argument (empty string with a corresponding %s if needed to prevent compiler warnings?). The ## is meant to compensate for that, according to the reading I did. I'm happy to back it out if it causes jenkins failures, but would like to at least try it as-is. -Rob You can upload it on any of the Jenkins jobs labeled ALPHA (login, click Build and fill out the form) to see if it breaks when applied to trunk on that OS/compiler. Amos
[PATCH] fix up external acl type dumping
This patch does four things: - adds a helper macro for format strings with external acl type config dumping. - uses that to add a missing type dumper for %%, which currently causes squid to FATAL if mgr:config is invoked. - refactors the SSL type dumping to use the macro as well, saving some redundant code - fixes a typo -case _external_acl_format::EXT_ACL_CA_CERT: -storeAppendPrintf(sentry, %%USER_CERT_%s, format-header); Seeking review, will land in a couple days if there is none :) -Rob === modified file 'src/external_acl.cc' --- src/external_acl.cc 2012-05-08 01:21:10 + +++ src/external_acl.cc 2012-06-14 08:58:33 + @@ -568,6 +568,10 @@ case _external_acl_format::EXT_ACL_##a: \ storeAppendPrintf(sentry, %%%s, #a); \ break +#define DUMP_EXT_ACL_TYPE_FMT(a, fmt, ...) \ +case _external_acl_format::EXT_ACL_##a: \ +storeAppendPrintf(sentry, fmt, ##__VA_ARGS__); \ +break #if USE_AUTH DUMP_EXT_ACL_TYPE(LOGIN); #endif @@ -592,28 +596,17 @@ DUMP_EXT_ACL_TYPE(PATH); DUMP_EXT_ACL_TYPE(METHOD); #if USE_SSL - -case _external_acl_format::EXT_ACL_USER_CERT_RAW: -storeAppendPrintf(sentry, %%USER_CERT); -break; - -case _external_acl_format::EXT_ACL_USER_CERTCHAIN_RAW: -storeAppendPrintf(sentry, %%USER_CERTCHAIN); -break; - -case _external_acl_format::EXT_ACL_USER_CERT: -storeAppendPrintf(sentry, %%USER_CERT_%s, format-header); -break; - -case _external_acl_format::EXT_ACL_CA_CERT: -storeAppendPrintf(sentry, %%USER_CERT_%s, format-header); -break; +DUMP_EXT_ACL_TYPE_FMT(USER_CERT_RAW, %%USER_CERT_RAW); +DUMP_EXT_ACL_TYPE_FMT(USER_CERTCHAIN_RAW, %%USER_CERTCHAIN_RAW); +DUMP_EXT_ACL_TYPE_FMT(USER_CERT, %%USER_CERT_%s, format-header); +DUMP_EXT_ACL_TYPE_FMT(CA_CERT, %%CA_CERT_%s, format-header); #endif #if USE_AUTH DUMP_EXT_ACL_TYPE(EXT_USER); #endif DUMP_EXT_ACL_TYPE(EXT_LOG); DUMP_EXT_ACL_TYPE(TAG); +DUMP_EXT_ACL_TYPE_FMT(PERCENT, ); default: fatal(unknown external_acl format error); break;
Re: [PATCH] fix up external acl type dumping
On 06/14/2012 03:06 AM, Robert Collins wrote: +#define DUMP_EXT_ACL_TYPE_FMT(a, fmt, ...) \ +case _external_acl_format::EXT_ACL_##a: \ +storeAppendPrintf(sentry, fmt, ##__VA_ARGS__); \ +break I do not see Squid using __VA_ARGS__ currently. Are you sure it is portable, especially in the ## context? If you have not tested this outside Linux, are you OK with us pulling out this patch if it breaks the build? If you are not sure, you can rework the macro to always use a single argument instead. The calls without a parameter in the format can add one and use an empty string (or you can have two macros). I do not like how this macro butchers the format code names. I could borderline agree with stripping the namespace, but stripping EXT_ACL_ prefix seems excessive. The prefix itself looks excessive (because the namespace already has external_acl in it!) but that is a different issue. I would prefer that the macro takes a real code name constant, without mutilating the name. There is an existing similar macro that does the same kind of mutilation, but there it is kind of justified because the name suffix is actually used. The true solution to this mess is to fix the names themselves, I guess. Cheers, Alex.
