From: | Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
---|---|
To: | Maxim Orlov <orlovmg(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Add SHELL_EXIT_CODE to psql |
Date: | 2023-01-09 18:36:12 |
Message-ID: | CADkLM=ekdTJwprfJZU43_Px6P8Y0ZTPNhc9j6K8BqE8snuoJBQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov <orlovmg(at)gmail(dot)com> wrote:
> Hi!
>
> In overall, I think we move in the right direction. But we could make code
> better, should we?
>
> + /* Capture exit code for SHELL_EXIT_CODE */
> + close_exit_code = pclose(fd);
> + if (close_exit_code == -1)
> + {
> + pg_log_error("%s: %m", cmd);
> + error = true;
> + }
> + if (WIFEXITED(close_exit_code))
> + exit_code=WEXITSTATUS(close_exit_code);
> + else if(WIFSIGNALED(close_exit_code))
> + exit_code=WTERMSIG(close_exit_code);
> + else if(WIFSTOPPED(close_exit_code))
> + exit_code=WSTOPSIG(close_exit_code);
> + if (exit_code)
> + error = true;
> I think, it's better to add spaces around middle if block. It will be easy
> to read.
> Also, consider, adding spaces around assignment in this block.
>
Noted and will implement.
> + /*
> + snprintf(exit_code_buf, sizeof(exit_code_buf), "%d",
> WEXITSTATUS(exit_code));
> + */
> Probably, this is not needed.
>
Heh. Oops.
> > 1. pg_regress now creates an environment variable called PG_OS_TARGET
> Maybe, we can use env "OS"? I do not know much about Windows, but I think
> this is kind of standard environment variable there.
>
I chose a name that would avoid collisions with anything a user might
potentially throw into their environment, so if the var "OS" is fairly
standard is a reason to avoid using it. Also, going with our own env var
allows us to stay in perfect synchronization with the build's #ifdef WIN32
... and whatever #ifdefs may come in the future for new OSes. If there is
already an environment variable that does that for us, I would rather use
that, but I haven't found it.
The 0001 patch is unchanged from last time (aside from anything rebasing
might have done).
>
Attachment | Content-Type | Size |
---|---|---|
v4-0001-Add-PG_OS_TARGET-environment-variable-to-enable-O.patch | text/x-patch | 1.1 KB |
v4-0002-Add-psql-variables-SHELL_ERROR-and-SHELL_EXIT_COD.patch | text/x-patch | 7.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2023-01-09 18:52:16 | Re: [PATCH] random_normal function |
Previous Message | Nathan Bossart | 2023-01-09 18:24:08 | Re: Allow +group in pg_ident.conf |