Re: [dev-serveez] OS X compatibility patches

[Julian Graham]

Hey ttn,

It's been a while since I've had access to a Mac OS X machine and cycles to spend on compatibility work, so please forgive my delayed reponse!

I understand the desire to split up patch 0005 into something more digestible, but I'm not sure I fully understand your suggestion on how to do it. Where should that local variable be introduced, and which counter within the `for' clause should it replace? In case it's faster for you to look at source code, here's my attempt at the "preparation" patch:

*SNIP*

@@ -451,9 +451,11 @@ 

 static void
 collect (void)
 {
   int numreqs = 16;
+  size_t iflen = sizeof (struct ifreq);
+
   struct ifconf ifc;
-  struct ifreq *ifr;
   struct ifreq ifr2;

   int n;
   int fd;

@@ -502,9 +504,10 @@ collect (void)
       break;
     }
 
-  ifr = ifc.ifc_req;
-  for (n = 0; n < ifc.ifc_len; n += sizeof (struct ifreq), ifr++)
+  for (n = 0; n < ifc.ifc_len; n += iflen)
     {
+      struct ifreq *ifr = ifc.ifc_req + n;
+      

*SNIP*

Does that get at what you were talking about? If so, great! I'll package it up into a real patch. If not, can you lead me through it a bit more explicitly?

Thanks,

Julian

On Tue, Feb 24, 2015 at 3:46 AM, Thien-Thi Nguyen <address@hidden> wrote:

() Julian Graham <address@hidden>
() Mon, 23 Feb 2015 23:05:28 -0500

   > [‘for’ transform]

   It took me a moment to remember, but:

   In the original code, `ifc.ifc_len' is always a multiple of
   `sizeof (struct ifreq)', and so `n' is incremented in the
   UPDATE as many times as there are interfaces in the
   buffer. `ifr' is just a pointer version of `n'.

   The new code acknowledges that the interface structures in
   `ifc' may have different lengths, and so `n' and `ifr' need
   to be incremented by the length of each structure in the
   buffer, an operation complex enough that I moved it out of
   the UPDATE part of the `for' loop. Think of it as handling a
   more general case than the original code.

Right.  I understand the generalization thrust.  The doubt
revolves around the precise timing of UPDATE wrt NON-OSX-PATH.
IIUC, before-patch, we have:

 for (INIT; GATE; UPDATE)
   {
     NON-OSX-PATH;
   }

which means that UPDATE is performed *after* NON-OSX-PATH, and
after-patch, we have:

 for (INIT; GATE; )
   {
 #if OSX
     OSX-PATH;
 #else
     UPDATE;
 #endif
     NON-OSX-PATH;
   }

which means that UPDATE is performed *before* NON-OSX-PATH.
Maybe i'm (still) missing something (coffee underflow error)?

I think i would be more inclined to accept a change that keeps
UPDATE where it is (in the ‘for’ "header" position) and instead
introduces a local variable for the entry length, w/ a default
constant value.  This would be the "preparation" patch.

The follow-on "payload" patch would then add OSX-PATH proper,
including dynamic update of that variable.  (Bonus points for
not requiring the #else branch.)

In this way, all steps can be more easily recognized as correct
by programmers of disparate abilities, experience and mindset.

--
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil