You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lukas Jirkovsky <l....@gmail.com> on 2017/05/03 20:05:01 UTC
[PATCH] gpg-agent storage - add support for /run based sockets V2
New version. Instead of reimplementing the gpg-agent logic, "gpgconf
--list-dir agent-socket" is used. If the gpgconf fails, the code falls
back to the original solution to stay compatible with old Gnupg
versions.
[[[
Find gpg-agent socket using gpgconf if possible.
This allows detection of socket with Gnupg >= 2.1.13
which changed the default socket path to /run/user/UID/gnupg
* subversion/libsvn_subr/gpg_agent.c
(find_gpgconf_agent_socket): new function to find
gpg-agent socket using gpgconf
(find_running_gpg_agent): use find_gpgconf_agent_socket
to detect socket when possible.
]]]
Re: [PATCH] gpg-agent storage - add support for /run based sockets V2
Posted by James McCoy <ja...@jamessan.com>.
On Wed, May 03, 2017 at 11:03:42PM +0200, Lukas Jirkovsky wrote:
> On 3 May 2017 at 22:35, Stefan Sperling <st...@elego.de> wrote:
> > On Wed, May 03, 2017 at 10:05:01PM +0200, Lukas Jirkovsky wrote:
> >> New version. Instead of reimplementing the gpg-agent logic, "gpgconf
> >> --list-dir agent-socket" is used. If the gpgconf fails, the code falls
> >> back to the original solution to stay compatible with old Gnupg
> >> versions.
I apologize for not looking into this more before recommending it. The
ability to specify a name to --list-dir is very recent (only since
2.1.14). All the other 2.1.x releases, and the entire 2.0.x series,
don't support an argument to --list-dir. They just print out all the
items.
$ gpgconf --list-dir
...
agent-socket:/run/user/1000/gnupg/S.gpg-agent
...
It's still a stable, easy to parse format, but it does require parsing
instead of just using the output directly.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: [PATCH] gpg-agent storage - add support for /run based sockets V2
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I have nothing to add to Stefan and James' review of functionality, so
here are a couple of style suggestions:
Lukas Jirkovsky wrote on Wed, May 03, 2017 at 23:03:42 +0200:
> +++ subversion/libsvn_subr/gpg_agent.c (working copy)
> @@ -225,6 +228,46 @@
> +find_gpgconf_agent_socket(apr_pool_t *pool)
> +{
> + apr_proc_t proc;
> + svn_stringbuf_t *line;
> + svn_error_t *err;
> + const char *gpgargv[] = { "gpgconf", "--list-dir", "agent-socket", NULL };
> +
You could add the second const here (after the asterisk) to remove the
need for the cast?
> + /* execute "gpgconf --list-dir agent-socket" */
> + err = svn_io_start_cmd3(&proc, NULL, "gpgconf", (const char* const*)gpgargv,
> + NULL, TRUE, FALSE, NULL, TRUE, NULL, FALSE, NULL,
> + pool);
Add "/* inherit */", "/* infile_pipe */", etc comments after the
booleans? (That's how we simulate Python's keyword arguments in C.)
Not trying to nitpick; just to make the code easier to read.
Cheers,
Daniel
Re: [PATCH] gpg-agent storage - add support for /run based sockets V2
Posted by Lukas Jirkovsky <l....@gmail.com>.
On 3 May 2017 at 22:35, Stefan Sperling <st...@elego.de> wrote:
> On Wed, May 03, 2017 at 10:05:01PM +0200, Lukas Jirkovsky wrote:
>> New version. Instead of reimplementing the gpg-agent logic, "gpgconf
>> --list-dir agent-socket" is used. If the gpgconf fails, the code falls
>> back to the original solution to stay compatible with old Gnupg
>> versions.
>
> Thanks Lukas, this patch looks good to me.
>
> I have just one minor stylistic nit:
> The local scratch_pool created in your new function is not necessary.
> This code could be simplified further by just using 'pool' throughout.
Hello Stefan,
thank you for comments. Attached is the updated version without scratch_pool.
> In any case, I would not object to having your patch committed as it is.
> The scratch_pool is just a small detail.
>
> It would be good if you could send patches as a unidiff in the future.
> 'svn diff' produces such output by default, and 'diff -u' works, too.
No problem, will do. I was just blindly following the Patch submission
guidelines from the Community Guide. I like the unified diff output
more as well.
Re: [PATCH] gpg-agent storage - add support for /run based sockets V2
Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 03, 2017 at 10:05:01PM +0200, Lukas Jirkovsky wrote:
> New version. Instead of reimplementing the gpg-agent logic, "gpgconf
> --list-dir agent-socket" is used. If the gpgconf fails, the code falls
> back to the original solution to stay compatible with old Gnupg
> versions.
Thanks Lukas, this patch looks good to me.
I have just one minor stylistic nit:
The local scratch_pool created in your new function is not necessary.
This code could be simplified further by just using 'pool' throughout.
In any case, I would not object to having your patch committed as it is.
The scratch_pool is just a small detail.
It would be good if you could send patches as a unidiff in the future.
'svn diff' produces such output by default, and 'diff -u' works, too.
Regards,
Stefan