The patch of glibc which allows the user to override the pfinetserver

Hi,

I modified the code of glibc, so the user can override the default
pfinet server by using the environment variable.
Here is the patch:
glibc-2.7-old/hurd/hurdsock.c 2008-06-21 01:38:30.040000000 +0200
glibc-2.7/hurd/hurdsock.c 2008-06-21 01:45:28.340000000 +0200
@@ -76,6 +76,30 @@

if (domain max_domain || servers[domain] == MACH_PRT_NULL)
{
+ char *sock_serv_path=NULL;
+ int len;
+ char *name=NULL;
+ char *np=NULL;
+ sock_serv_path=getenv("SCK_SERV_PATH");
+ if(sock_serv_path == NULL)
+ sock_serv_path=_SERVERS_SCKET;
+ len=strlen(sock_serv_path);
+ name=(char *)malloc(len+100);
+ if(name == NULL)
+ __libc_fatal ("hurd: Can't allocate the socket server path\n");
+ np=&name[len+100];
+ * = '\0';
+ np = _itoa (domain, np, 10, 0);
+ * = '/';
+
+ np -= len;
+ memcpy (np, sock_serv_path , len);
+
+ server = __file_name_lookup (np, 0, 0);
+ if (domain <= max_domain)
+ servers[domain] = server;
+ free(name);
+#if 0
char name[sizeof (_SERVERS_SCKET) + 100];
char *np = &name[sizeof (name)];
* = '\0';
@@ -86,6 +110,7 @@
server = __file_name_lookup (np, 0, 0);
if (domain <= max_domain)
servers[domain] = server;
+#endif
}
else
server = servers[domain];

Any comments?

Best,
Zheng Da

Neal H. Walfield wrote:
This works in the case where you want to override all pf servers.
This case is important. Also important is the ability to override a
single pf server in a similar manner.
>
A couple comments on the code: please follow the GNU coding standards.
Second, I assume that you left the #if 0 in because you are requesting
review. Make sure that you remove this when you really intend to
submit the patch. Finally, when that happens, make sure to include a
change log entry.
>
Neal

I wonder if it's really necessary to override a single pf server.
If more network protocols are implemented, we have to provide more
environment variables for the servers.
the other hand, if we override the path of all socket servers, we can
create a symbolic link to the default server which we don't want to
override.
Anyway, I provide two more environment variables SCK_INET_SERV_PATH and
SCK_LCAL_SERV_PATH for the pfinet server and pflocal server, respectively.
I try to make my code meet the GNU coding standard. If it doesn't, could
you point it out.
The GNU coding standard is a bit long, so I'm not sure if my code meets
all GNU standards.

Here is the patch of the second version

glibc-2.7-old/hurd/hurdsock.c 2008-06-21 01:38:30.000000000 +0200
/glibc-2.7/hurd/hurdsock.c 2008-06-24 12:55:23.000000000 +0200
@@ -76,16 +76,39 @@

if (domain max_domain || servers[domain] == MACH_PRT_NULL)
{
- char name[sizeof (_SERVERS_SCKET) + 100];
- char *np = &name[sizeof (name)];
- * = '\0';
- np = _itoa (domain, np, 10, 0);
- * = '/';
- np -= sizeof (_SERVERS_SCKET) - 1;
- memcpy (np, _SERVERS_SCKET, sizeof (_SERVERS_SCKET) - 1);
+ char *sock_serv_path = NULL;
+ int len;
+ char *name = NULL;
+ char *np = NULL;
+ if ((domain == AF_INET
+ && (sock_serv_path = getenv ("SCK_INET_SERV_PATH")))
+ || (domain == AF_LCAL
+ && (sock_serv_path = getenv ("SCK_LCAL_SERV_PATH"))))
+ {
+ np = sock_serv_path;
+ }
+ else
+ {
+ sock_serv_path = getenv ("SCK_SERV_PATH");
+ if (sock_serv_path == NULL)
+ sock_serv_path = _SERVERS_SCKET;
+ len = strlen (sock_serv_path);
+ name = (char *)malloc (len + 100);
+ if (name == NULL)
+ __libc_fatal ("hurd: Can't allocate the socket server path\n");
+ np = &name[len + 100];
+ * = '\0';
+ np = _itoa (domain, np, 10, 0);
+ * = '/';
+ np -= len;
+ memcpy (np, sock_serv_path , len);
+ }
+
server = __file_name_lookup (np, 0, 0);
if (domain <= max_domain)
- servers[domain] = server;
+ servers[domain] = server;
+ if (name)
+ free(name);
}
else
server = servers[domain];

Best,
Zheng Da

zhengda, le Thu 26 Jun 2008 19:31:12 +0200, a :
>Hm Something seems wrong with the indentation here -- but it looks as
>if it has been already wrong in the original code Can you fix that
>please?
>
>, and of course you still need a changelog entry. There is a section
>in the GCS on that as well

Where should I write the changelog? in the patch?

Put it at the top of the patch yes. Make sure you have read the GNU
documentation about ChangeLogs: they are meant to be a mere summary of
what modifications the patch does, not of its goal. Read previous
ChangeLog entries for samples.

Samuel

Hi,

Wed, Jul 02, 2008 at 08:42:43PM +0200, zhengda wrote:

The new version of the patch is below.

Hm, what about dropping/replacing the "_PATH" bit, as discussed in the
other subthread?

I wonder if I can use __snprintf(). The code in the original glibc
doesn't use it.

I'm not a glibc hacker; but I would be very surprised if it was a
problem

+ __snprintf (sock_serv_env_name, 30, "SCK_SERV_%d_PATH", domain);

I'd say use "sizeof sock_serv_env_name", rather than hard-coding 30, for
better robustness

-antrik-

Hello,

+ char sock_serv_env_name[30];

To avoid a fixed-sized buffer you could use __asprintf.

+ name = (char *)malloc (len + 100);

Use __asprintf here too.

- servers[domain] = server;
+ servers[domain] = server;

Spurious hook, please try to minimize these.

About the ChangeLog entry, it needs to be more precise: add the
environment variable and macro _names_ since that's what people would
grep for.

Samuel