This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8021

chg: switch to using global `environ` instead of envp from main
AbandonedPublic

Authored by spectral on Jan 27 2020, 7:57 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

My followup patch wants to modify the environment before we call setenv on the
chg server process (to add items), and that won't be handled properly if we use
envp (which isn't modified by putenv).

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

spectral created this revision.Jan 27 2020, 7:57 PM
pulkit added a subscriber: pulkit.Jan 28 2020, 6:57 AM

I don't know about the code which this series touch, but since D7550 already made to 5.3rc, should this series be targeted for stable branch?

yuja added a subscriber: yuja.Jan 28 2020, 11:10 AM

{

	if (getenv("CHGDEBUG"))
		enabledebugmsg();

@@ -429,7 +431,7 @@

		hgc = connectcmdserver(&opts);
		if (!hgc)
			abortmsg("cannot open hg client");
  • hgc_setenv(hgc, envp);

+ hgc_setenv(hgc);

I prefer passing in (const char**)environ to hgc_setenv().

spectral updated this revision to Diff 19655.Jan 28 2020, 1:56 PM
spectral added a comment.Jan 28 2020, 1:57 PM

I prefer passing in (const char**)environ to hgc_setenv().

Done.

spectral updated this revision to Diff 19657.Jan 28 2020, 1:57 PM
martinvonz added a subscriber: martinvonz.Jan 28 2020, 4:36 PM

Queuing based on @yuja's review.

martinvonz added a comment.Jan 28 2020, 4:38 PM
In D8021#118342, @martinvonz wrote:

Queuing based on @yuja's review.

Actually, since there's no point with this patch without the child, I'll wait until @spectral and @yuja have agreed what to do with that one first.

spectral abandoned this revision.Feb 4 2020, 4:57 PM

Revision Contents
Changeset List

PathPackages
Mcontrib/chg/chg.c (6 lines)
Mcontrib/chg/hgclient.h (2 lines)
Mcontrib/chg/hgclient.c (8 lines)

Diff 19650

contrib/chg/chg.c

#include "hgclient.h" #include "hgclient.h"
#include "procutil.h" #include "procutil.h"
#include "util.h" #include "util.h"
#ifndef PATH_MAX #ifndef PATH_MAX
#define PATH_MAX 4096 #define PATH_MAX 4096
#endif #endif
extern char **environ;
struct cmdserveropts { struct cmdserveropts {
char sockname[PATH_MAX]; char sockname[PATH_MAX];
char initsockname[PATH_MAX]; char initsockname[PATH_MAX];
char redirectsockname[PATH_MAX]; char redirectsockname[PATH_MAX];
size_t argsize; size_t argsize;
const char **args; const char **args;
}; };
static void execoriginalhg(const char *argv[]) static void execoriginalhg(const char *argv[])
{ {
debugmsg("execute original hg"); debugmsg("execute original hg");
if (execvp(gethgcmd(), (char **)argv) < 0) if (execvp(gethgcmd(), (char **)argv) < 0)
abortmsgerrno("failed to exec original hg"); abortmsgerrno("failed to exec original hg");
} }
int main(int argc, const char *argv[], const char *envp[]) int main(int argc, const char *argv[])
{ {
if (getenv("CHGDEBUG")) if (getenv("CHGDEBUG"))
enabledebugmsg(); enabledebugmsg();
if (!getenv("HGPLAIN") && isatty(fileno(stderr))) if (!getenv("HGPLAIN") && isatty(fileno(stderr)))
enablecolor(); enablecolor();
if (getenv("CHGINTERNALMARK")) if (getenv("CHGINTERNALMARK"))
} }
hgclient_t *hgc; hgclient_t *hgc;
size_t retry = 0; size_t retry = 0;
while (1) { while (1) {
hgc = connectcmdserver(&opts); hgc = connectcmdserver(&opts);
if (!hgc) if (!hgc)
abortmsg("cannot open hg client"); abortmsg("cannot open hg client");
hgc_setenv(hgc, envp); hgc_setenv(hgc);
const char **insts = hgc_validate(hgc, argv + 1, argc - 1); const char **insts = hgc_validate(hgc, argv + 1, argc - 1);
int needreconnect = runinstructions(&opts, insts); int needreconnect = runinstructions(&opts, insts);
free(insts); free(insts);
if (!needreconnect) if (!needreconnect)
break; break;
hgc_close(hgc); hgc_close(hgc);
if (++retry > 10) if (++retry > 10)
abortmsg("too many redirections.\n" abortmsg("too many redirections.\n"

contrib/chg/hgclient.h

pid_t hgc_peerpgid(const hgclient_t *hgc); pid_t hgc_peerpgid(const hgclient_t *hgc);
pid_t hgc_peerpid(const hgclient_t *hgc); pid_t hgc_peerpid(const hgclient_t *hgc);
const char **hgc_validate(hgclient_t *hgc, const char *const args[], const char **hgc_validate(hgclient_t *hgc, const char *const args[],
size_t argsize); size_t argsize);
int hgc_runcommand(hgclient_t *hgc, const char *const args[], size_t argsize); int hgc_runcommand(hgclient_t *hgc, const char *const args[], size_t argsize);
void hgc_attachio(hgclient_t *hgc); void hgc_attachio(hgclient_t *hgc);
void hgc_setenv(hgclient_t *hgc, const char *const envp[]); void hgc_setenv(hgclient_t *hgc);
#endif /* HGCLIENT_H_ */ #endif /* HGCLIENT_H_ */

contrib/chg/hgclient.c

#include <sys/stat.h> #include <sys/stat.h>
#include <sys/un.h> #include <sys/un.h>
#include <unistd.h> #include <unistd.h>
#include "hgclient.h" #include "hgclient.h"
#include "procutil.h" #include "procutil.h"
#include "util.h" #include "util.h"
extern char **environ;
enum { CAP_GETENCODING = 0x0001, enum { CAP_GETENCODING = 0x0001,
CAP_RUNCOMMAND = 0x0002, CAP_RUNCOMMAND = 0x0002,
/* cHg extension: */ /* cHg extension: */
CAP_ATTACHIO = 0x0100, CAP_ATTACHIO = 0x0100,
CAP_CHDIR = 0x0200, CAP_CHDIR = 0x0200,
CAP_SETENV = 0x0800, CAP_SETENV = 0x0800,
CAP_SETUMASK2 = 0x1000, CAP_SETUMASK2 = 0x1000,
CAP_VALIDATE = 0x2000, CAP_VALIDATE = 0x2000,
} }
/*! /*!
* Update server's environment variables * Update server's environment variables
* *
* @param envp list of environment variables in "NAME=VALUE" format, * @param envp list of environment variables in "NAME=VALUE" format,
* terminated by NULL. * terminated by NULL.
*/ */
void hgc_setenv(hgclient_t *hgc, const char *const envp[]) void hgc_setenv(hgclient_t *hgc)
{ {
assert(hgc && envp); assert(hgc && environ);
if (!(hgc->capflags & CAP_SETENV)) { if (!(hgc->capflags & CAP_SETENV)) {
return; return;
} }
packcmdargs(&hgc->ctx, envp, /*argsize*/ -1); packcmdargs(&hgc->ctx, (const char**)environ, /*argsize*/ -1);
writeblockrequest(hgc, "setenv"); writeblockrequest(hgc, "setenv");
} }