Re: Confusing definitions and declarations of mig_dealloc_reply_port()

[Svante Signell]

Diego,
Cc: bug-hurd.

On Tue, 2015-11-03 at 15:55 -0300, Diego Nieto Cid wrote:
> Hi
> 
> 2015-11-03 9:51 GMT-03:00 Svante Signell <svante.signell@gmail.com>:
> >
> > Hello,
> >
> 
> Definition, declaration and usages are all consistent, they pass and
> receive one argument.
> The argument being unsed is just an implementation detail; earlier
> versions may had used
> it but was never removed [*]....
> 
> I guess what's important is that it's consistent with how
> mig_get_reply_port gives the port
> to its users, right?
> 
> >   __mig_dealloc_reply_port (MACH_PORT_NULL);
> >
> 
> This use case would be broken by...
> 
> ...this patch. arg would be MACH_PORT_NULL and no refs would get
> released, leaking the reply port.

Well, the following patch would resolve that, right? Maybe overkill
though.

Index: glibc-2.19/sysdeps/mach/hurd/mig-reply.c
===================================================================
--- glibc-2.19.orig/sysdeps/mach/hurd/mig-reply.c
+++ glibc-2.19/sysdeps/mach/hurd/mig-reply.c
@@ -39,7 +39,16 @@ weak_alias (__mig_get_reply_port, mig_ge
 void
 __mig_dealloc_reply_port (mach_port_t arg)
 {
+#if 1
+  mach_port_t port;
+  if (arg == MACH_PORT_NULL)
+    port = __hurd_local_reply_port;
+  else
+    port = arg;
+#else
   mach_port_t port = __hurd_local_reply_port;
+#endif
+  assert (port == arg || arg == MACH_PORT_NULL);
   __hurd_local_reply_port = MACH_PORT_NULL;    /* So the mod_refs
RPC won't use it.  */
 
   if (MACH_PORT_VALID (port))

> I'd suggest to assert (port == arg || arg == MACH_PORT_NULL) just to
> be sure users don't expect
> other port to be deallocated, which would be a bug.

See above.

> > Another solution could be to remove the argument in the
> > mig_dealloc_reply_port definition, but that would require a change
> also
> > to mig.
> >
> 
> [*]...probably because of this :)

Why not change mig, is it holy?