On Thu, May 19, 2011 at 1:37 PM, Rafael Gieschke <rafael@xxxxxxxxxxx> wrote: > > If NO_GETPASS is set, getpass is provided in compat/getpass.c from > https://github.com/CyanogenMod/android_external_dropbear/raw/master/netbsd_getpass.c > (getpass was renamed to gitgetpass). Nice. But I can't help to think that this implementation of getpass looks a bit heavy, especially since we already have our own getpass implementation in compat/mingw.c. Do we really need two implementations? Wouldn't it be better to factor out the mingw-version to a separate source file, and then improve it? Windows doesn't have /dev/tty, but the logic in this version handles that by using stdin/stderr instead. The signal-stuff has a comment that indicates it might not even be correct. tcgetattr/tcsetattr isn't supported on Windows, but it's not needed if we use getch (as the version in compat/mingw.c does). POSIX/curses getch respects the echo-setting, while Windows getch never echo. Given the information above, it sounds to me like we can enhance the version we already have to behave as it should also on non-Windows platforms, without having to maintain two versions. But it might not be worth it, given the simplicity of the Windows version and the increased dependency of curses, I dunno. As a Windows-guy, I'm not entirely comfortable with putting something in compat/[function-name].c that is only portable to POSIX-platforms. This is not the current trend, but that might not be a conscious choice. > diff --git a/compat/getpass.c b/compat/getpass.c > new file mode 100644 > index 0000000..e13f29f > --- /dev/null > +++ b/compat/getpass.c > @@ -0,0 +1,114 @@ > +/* $NetBSD: getpass.c,v 1.15 2003/08/07 16:42:50 agc Exp $ */ > + > +/* > + * Copyright (c) 1988, 1993 > + * The Regents of the University of California. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of the University nor the names of its contributors > + * may be used to endorse or promote products derived from this software > + * without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#if 0 > +#include <sys/cdefs.h> > +#if defined(LIBC_SCCS) && !defined(lint) > +#if 0 > +static char sccsid[] = "@(#)getpass.c 8.1 (Berkeley) 6/4/93"; > +#else > +__RCSID("$NetBSD: getpass.c,v 1.15 2003/08/07 16:42:50 agc Exp $"); > +#endif > +#endif /* LIBC_SCCS and not lint */ We usually don't keep dead code around. Since you've already edited the file, perhaps you should just delete those lines? > + > +#if 0 > +#ifdef __weak_alias > +__weak_alias(getpass,_getpass) > +#endif > +#endif Same here. > + > +char * > +gitgetpass(prompt) > + const char *prompt; > +{ > + struct termios term; > + int ch; > + char *p; > + FILE *fp, *outfp; > + int echo; > + static char buf[_PASSWORD_LEN + 1]; Is _PASSWORD_LEN portable? A google search of "site:opengroup.org _PASSWORD_LEN " returns nothing... The getpass implementation in compat/mingw.c uses a strbuf, so it doesn't have a limit on the password-length. Perhaps that would be an improvement here? > + sigset_t oset, nset; > + > +#if 0 > + _DIAGASSERT(prompt != NULL); > +#endif Again, dead code. > + > + /* > + * read and write to /dev/tty if possible; else read from > + * stdin and write to stderr. > + */ > + if ((outfp = fp = fopen(_PATH_TTY, "w+")) == NULL) { Is _PATH_TTY portable? A google search of "site:opengroup.org _PATH_TTY" returns nothing... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html