Excerpts from Yuya Nishihara's message of 2016-11-13 23:18:10 +0900:
> On Fri, 11 Nov 2016 21:14:37 +0000, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <qu...@fb.com>
> > # Date 1478898677 0
> > # Fri Nov 11 21:11:17 2016 +0000
> > # Node ID 4ccb6bcaf25a741d3a0d8abd6c674573ef76069a
> > # Parent 98761d64eaaf67f3bdb99f3f80a57910e2624b78
> > # Available At https://bitbucket.org/quark-zju/hg-draft
> > # hg pull https://bitbucket.org/quark-zju/hg-draft -r
> > 4ccb6bcaf25a
> > osutil: implement setprocname to set process title for some platforms
>
> Seems fine. We can't guarantee argv[i] is null terminated for all i, but
> that seems less dangerous since the last argv[i] always has '\0'.
You mean if some other code modifies "argv"? Seems fine - we may just think
the buffer is smaller.
> A few nits.
>
> > --- a/mercurial/osutil.c
> > +++ b/mercurial/osutil.c
> > @@ -728,4 +728,59 @@ bail:
> >
> > #endif /* CMSG_LEN */
> > +
> > +/* Check what setprocname method we can use:
> > + * a. use setproctitle if it's available - works in FreeBSD
> > + * b. rewrite **argv buffer in place - works in Linux and OS X
> > + * Note: for "b.", we need Python 2, because Python 3's Py_GetArgcArgv
> > returns
> > + * the copied wchar_t **argv, not the original char **argv. */
> > +#if defined(HAVE_SETPROCTITLE)
> > +#define setprocname_method 'a'
> > +#elif (defined(__linux__) || defined(__APPLE__)) && PY_MAJOR_VERSION == 2
> > +#define setprocname_method 'b'
> > +#else
> > +#undef setprocname_method
> > +#endif
>
> Lowercase macro name and char-type value seem not common. Can you change them
> to old-school macros such as SETPROCNAME_USE_SETPROCTITLE?
I was trying to write shorter code and avoid some name conflicts. It seems
the old style is not that bad.
> > +#ifdef setprocname_method
> > +static PyObject *setprocname(PyObject *self, PyObject *args)
> > +{
> > + const char *name = NULL;
> > + if (!PyArg_ParseTuple(args, "s", &name))
> > + return NULL;
> > +
> > +#if 'a' == setprocname_method
> > + setproctitle("%s", name);
> > +#elif 'b' == setprocname_method
> > + static char *argvstart = NULL;
> > + static size_t argvsize = 0;
>
> Strictly speaking, these declarations should be moved to the block head. (But
> almost all modern compilers but for MSVC would allow mixed declarations.)
Or we can put them in a "{}" scope - seems more readable.
> > + if (argvstart == NULL) {
> > + int argc = 0, i;
> > + char **argv = NULL;
> > + extern void Py_GetArgcArgv(int *argc, char ***argv);
> > + Py_GetArgcArgv(&argc, &argv);
> > +
> > + /* Check the memory we can use. Typically, argv[i] and
> > + * argv[i+1] are separated by a NULL character. */
> > + if (argc > 0) {
> > + argvstart = argv[0];
> > + argvsize = strlen(argv[0]) + 1;
> > + for (i = 1; i < argc; ++i) {
> > + if (argv[i] == argvstart + argvsize)
> > + argvsize += strlen(argv[i]) + 1;
> > + }
> > + }
>
> Perhaps this loop can start with argvstart = argv[0], argvsize = 0, i = 0.
I'll find a way to make it short and possibly handle the "argv was rewritten
by others" case.
> > + }
> > +
> > + if (argvstart && argvsize > 1) {
> > + int n = snprintf(argvstart, argvsize - 1, "%s", name);
>
> IIRC, snprintf() takes size including the last null byte and null termination
> is guaranteed.
Embarrassing. It turns out I have been misreading the manpage for years.
> > + if (n >= 0 && (size_t)n < argvsize)
> > + memset(argvstart + n, 0, argvsize - n);
> > + }
> > +#endif
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
