Re: pg_hba options parsing

Bruce Momjian wrote:
> OK, a few comments:
>
> This should have spaces:
>
> ! port->hba->ldapprefix?port->hba->ldapprefix:"",

Fixed. Though I guess pgindent would've fixed it eventually otherwise.

> This is missing 'do' or something:
>
> + #define MANDATORY_AUTH_ARG(argvar, argname, authname) \
> + if (argvar == NULL) {\
> + ereport(LOG, \
> + (errcode(ERRCODE_CONFIG_FILE_ERROR), \
> + errmsg("authentication method '%s' requires argument '%s' to be set", \
> + authname, argname), \
> + errcontext("line %d of configuration file \"%s\"", \
> + line_num, HbaFileName))); \
> + goto hba_other_error; \
> + } while (0);

Wow.Amazing that it actually compiles and work. I guess it treats the
while(0) as a separate statement completely.

The correct fix is, AFAICS, to remove the while(0).

> Seems we are losing the sscanf(), which is good.

Yes, that is one of the goals :-)

If there are no further comments I will go ahead and commit this (with
the above fixes) within a couple of days.

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Bruce Momjian wrote:
>> This is missing 'do' or something:
>>
>> + #define MANDATORY_AUTH_ARG(argvar, argname, authname) \
>> + if (argvar == NULL) {\
>> + ereport(LOG, \
>> + (errcode(ERRCODE_CONFIG_FILE_ERROR), \
>> + errmsg("authentication method '%s' requires argument '%s' to be set", \
>> + authname, argname), \
>> + errcontext("line %d of configuration file \"%s\"", \
>> + line_num, HbaFileName))); \
>> + goto hba_other_error; \
>> + } while (0);

> Wow.Amazing that it actually compiles and work. I guess it treats the
> while(0) as a separate statement completely.

> The correct fix is, AFAICS, to remove the while(0).

Absolutely not! The reason for using do/while in this sort of situation
is to make sure that the "if" can't get matched up to an "else" in code
following the macro. Without do/while this macro will be a loaded
foot-gun.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Bruce Momjian wrote:
>>> This is missing 'do' or something:
>>>
>>> + #define MANDATORY_AUTH_ARG(argvar, argname, authname) \
>>> + if (argvar == NULL) {\
>>> + ereport(LOG, \
>>> + (errcode(ERRCODE_CONFIG_FILE_ERROR), \
>>> + errmsg("authentication method '%s' requires argument '%s' to be set", \
>>> + authname, argname), \
>>> + errcontext("line %d of configuration file \"%s\"", \
>>> + line_num, HbaFileName))); \
>>> + goto hba_other_error; \
>>> + } while (0);
>
>> Wow.Amazing that it actually compiles and work. I guess it treats the
>> while(0) as a separate statement completely.
>
>> The correct fix is, AFAICS, to remove the while(0).
>
> Absolutely not! The reason for using do/while in this sort of situation
> is to make sure that the "if" can't get matched up to an "else" in code
> following the macro. Without do/while this macro will be a loaded
> foot-gun.

Oh, didn't think of that. I just thought of the braces part, which was
"solved" by if. Thanks for clearing that up.

Ok, will add back do/while instead.

//Magnus