[PATCH] Add inside-Emacs mode to GUI pinentry programs
ueno at gnu.org
Tue Jun 2 10:56:44 CEST 2015
Thanks for the thorough review, responses are inlined below.
By the way, on second thoughts, I ended up with a specialized protocol
for this, to avoid unnecessary character escaping and encoding
conversion (and also reconnecting to a socket every time). The protocol
is actually a subset of the Pinentry Assuan protocol, and implemented in
pinentry.el, which shall be included in Emacs, maybe under lisp/net/ or
I'm attaching the new patch and pinentry.el. To test, use "M-x
pinentry-start" instead of "M-x server-start".
"Neal H. Walfield" <neal at walfield.org> writes:
>> +if test "$pinentry_emacs" = "maybe"; then
>> + AC_MSG_CHECKING([if Unix domain socket is supported])
> I think we should check for != "no" here. If pinentry_emacs is yes,
> but we don't support unix domain sockets, then it would be better to
> error out here than when compiling the code. Do you agree?
Yes, exactly. Fixed.
>> +#include <assert.h>
> As far as I can tell, this include is gratuitious.
>> + if (strlen (tmpdir) + strlen (socket_name) + 10 + 2
>> + >= sizeof (emacs_server_address.sun_path))
>> + sprintf (emacs_server_address.sun_path, "%s/emacs%lu/%s",
>> + tmpdir, (unsigned long) uid, socket_name);
> I think the check above is wrong. It should be something like:
> if (strlen (tmpdir) + strlen("/emacs") + 10 + strlen("/")
> + strlen (socket_name) + 1
> >= sizeof (emacs_server_address.sun_path))
Right, thanks for pointing that out. Fixed using the correct formula.
>> + if (connect (s, (struct sockaddr *) &emacs_server_address,
>> + strlen (emacs_server_address.sun_path) + 2) < 0)
> Why don't you just use 'sizeof (emacs_server_address)' here? I really
> don't like the magic number 2 here.
Fixed using SUN_LEN from GnuPG.
>> + if (strprefix ("-print ", p))
> Does it make sense to emit a warning if there is a line with an
> unrecognized prefix? As far as I can see, lines with unknown commands
> are currently ignored.
Yes, added warnings.
>> + sprintf (command, "(" CALLBACK_NAME " \"%s\" %s)",
>> + name, quoted_value);
>> + if (quoted_value != nil)
>> + free (quoted_value);
> Here's another instance where I'd strongly prefer something like
Yes. Those allocation and copying are no longer necessary with the new
>> + quoted_command = build_command (name, local_value);
>> + if (local_value != value)
>> + free (local_value);
> I think this comparison is useless. If value is not NULL, then we
> call pinentry_utf8_to_local, which always allocates a new buffer.
Good point. Likewise to the above, the extra allocation is not needed
> Does emacs not support setting custom button labels?
Yes, it could. However, for a password query, labels would be too
verbose to be shown in the minibuffer. So, in pinentry.el, they are
only used for a confirmation query.
> If pe->prompt is NULL, then you should fallback to pe->default_prompt.
> If you have time, it would be nice to add support for the external
> password manager checkbox. In the very least, please note the lack of
> this support with an XXX comment in the source code.
Sure, added a comment.
>> + if (pe->repeat_passphrase)
>> + pe->repeat_okay = 1;
> Why are you setting this if the user didn't actually enter the
> password twice?
I implemented the repeat feature in the new patch.
>> +do_confirm (pinentry_t pe)
>> + set_value (pe, "SETPROMPT", pe->prompt);
> Again, please fallback to pe->default_prompt.
> Any reason, there is no support for custom button labels? Also, what
> about support for the other button? If this is not yet possible,
> please note it with an XXX comment.
>> -#if defined FALLBACK_CURSES || defined PINENTRY_CURSES || defined
>> +#if defined FALLBACK_CURSES || defined INSIDE_EMACS || defined
>> PINENTRY_CURSES || defined PINENTRY_GTK
>> char *
>> pinentry_utf8_to_local (const char *lc_ctype, const char *text)
> I think we should check if PINENTRY_EMACS is defined, not INSIDE_EMACS
I reverted this change since the patch no longer uses
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 33333 bytes
Desc: not available
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
More information about the Gnupg-devel