Re: [PATCH] Add use of asprintf() - Mailing list pgsql-hackers
| From | Asif Naeem |
|---|---|
| Subject | Re: [PATCH] Add use of asprintf() |
| Date | |
| Msg-id | CAEB4t-M3yik0H4CK5g96ZnAmMkzfTb_5pobu1RP+ZBPeZmWzQg@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] Add use of asprintf() (Alvaro Herrera <alvherre@2ndquadrant.com>) |
| Responses |
Re: [PATCH] Add use of asprintf()
Re: [PATCH] Add use of asprintf() Re: [PATCH] Add use of asprintf() |
| List | pgsql-hackers |
Hi,
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 ?
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.
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;
}
/*
* 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 <alvherre@2ndquadrant.com> wrote:
Peter Eisentraut wrote:Yeah, it's a very nice cleanup.
> The attached patch should speak for itself.Looks good to me, except that pg_asprintf seems to be checking ret
> 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.
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
pgsql-hackers by date: