On Tue, Sep 17, 2013 at 3:13 PM, Asif Naeem <anaeem...@gmail.com> wrote:
> Hi,
>
> I did put some time review the patch, please see my findings below i.e.
>
> 1. It seems that you have used strdup() on multiple places in the patch,
> e.g. in the below code snippet is it going to lead crash if newp->ident is
> NULL because of strdup() failure ?
>
> static EPlan *
>> find_plan(char *ident, EPlan **eplan, int *nplans)
>> {
>> ...
>> newp->ident = strdup(ident);
>> ...
>> }
>
>
> 2. Can we rely on return value of asprintf() instead of recompute length
> as strlen(cmdbuf) ?
>
> if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS '", loid) <
>> 0)
>> return fail_lo_xact("\\lo_import", own_transaction);
>> bufptr = cmdbuf + strlen(cmdbuf);
>
>
> 3. There seems naming convention for functions like pfree (that seems
> similar to free()), pstrdup etc; but psprintf seems different than sprintf.
> Can't we use pg_asprintf instead in the patch, as it seems that both
> (psprintf and pg_asprintf) provide almost same functionality ?
>
> 4. It seems that "ret < 0" need to be changed to "rc < 0" in the following
> code i.e.
>
> int
>> pg_asprintf(char **ret, const char *format, ...)
>> {
>> va_list ap;
>> int rc;
>> va_start(ap, format);
>> rc = vasprintf(ret, format, ap);
>> va_end(ap);
>> if (ret < 0)
>> {
>> fprintf(stderr, _("out of memory\n"));
>> exit(EXIT_FAILURE);
>> }
>> return rc;
>> }
>
>
It seems this point is already mentioned by Alvaro. Thanks.
>
> 5. It seems that in the previously implementation, error handling was not
> there, is it appropriate here that if there is issue in duplicating
> environment, quit the application ? i.e.
>
> /*
>> * Handy subroutine for setting an environment variable "var" to "val"
>> */
>> static void
>> doputenv(const char *var, const char *val)
>> {
>> char *s;
>> pg_asprintf(&s, "%s=%s", var, val);
>> putenv(s);
>> }
>
>
>
> Please do let me know if I missed something or more info is required.
> Thank you.
>
> Regards,
> Muhammad Asif Naeem
>
>
> On Tue, Sep 17, 2013 at 1:31 AM, Alvaro Herrera
> <alvhe...@2ndquadrant.com>wrote:
>
>> Peter Eisentraut wrote:
>>
>> > The attached patch should speak for itself.
>>
>> Yeah, it's a very nice cleanup.
>>
>> > I have supplied a few variants:
>> >
>> > - asprintf() is the standard function, supplied by libpgport if
>> > necessary.
>> >
>> > - pg_asprintf() is asprintf() with automatic error handling (like
>> > pg_malloc(), etc.)
>> >
>> > - psprintf() is the same idea but with palloc.
>>
>> Looks good to me, except that pg_asprintf seems to be checking ret
>> instead of rc.
>>
>> Is there a reason for the API discrepancy of pg_asprintf vs. psprintf?
>> I don't see that we use the integer return value anywhere. Callers
>> interested in the return value can use asprintf directly (and you have
>> already inserted callers that do nonstandard things using direct
>> asprintf).
>>
>> --
>> Álvaro Herrera http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>
