[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug #33138] .PARLLELSYNC enhancement with patch

From: Frank Heckenbach
Subject: [bug #33138] .PARLLELSYNC enhancement with patch
Date: Sat, 29 Dec 2012 00:58:29 +0000
User-agent: Mozilla/5.0 (X11; U; Linux i686; de-DE; rv: Gecko/20110701 Iceweasel/3.5.16 (like Firefox/3.5.16)

Follow-up Comment #2, bug #33138 (project make):

Some remarks on the current patch:


+  /* Check status of stdout and stderr before hooking up temp files. */
+  o_ok = STREAM_OK(stdout);
+  e_ok = STREAM_OK(stderr);

Why do you check these for each recipe and not just once like you do for
combined_output? -- Actually, you do check STREAM_OK already in the
combined_output checks, so can't you store the results? Sure, it saves just 2
system calls, but it seems to me these things belong together and thus should
be treated the same, if only to make the code clearer. In the end, all these
checks combined yield four possibilities, stdout only, stderr only, both
separate, both combined. It seems confusing to pass one part of the
information as a parameter and recheck the rest.

+  /* The tmpfile() function returns a FILE pointer but those can be in
+     limited supply, so we immediately dup its file descriptor and keep
+     only that, closing the FILE pointer.  */

On POSIX, you could perhaps use mkstemp() which returns an FD directly.


+  ssize_t nleft, nwrite;
+  char buffer[8192];
+       while (nleft > 0)
+       {
+         EINTRLOOP(nwrite, write(to_fd, buffer, nleft));
+         [...]
+         nleft -= nwrite;
+       }

If the loop runs more than once, it will write the first part of the buffer
repeatedly. You need an offset that is increased by nwrite.


+  fl.l_pid = getpid();
+  fl.l_start = fl.l_pid; /* lock just one byte according to pid */
+  fl.l_len = 1;

Why do you lock a *different* byte for each PID? The purpose of a semaphore is
mutual exclusion, so I think you should lock the same byte every time.

Furthermore, you don't need to set l_pid to the current PID. It's an output
field used by F_GETLK to return the PID of the process holding the lock.


I'm not too happy with it being triggered by a special target (.PARALLELSYNC).
Why should the Makefile dictate how the user sees the output? Perhaps some
users want immediate feedback (as mentioned by Eli Zaretskii in the original
discussion) while others prefer delayed, but properly grouped output. So why
not use a command-line option that each user can set to their preferences,
just like they can with "-j" itself?


Reply to this item at:


  Nachricht gesendet von/durch Savannah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]