Re: [bug #33138] .PARLLELSYNC enhancement with patch

[Frank Heckenbach]

Eli Zaretskii wrote:

> > : Btw, there will be other side effects, at least on non-Posix
> > : platforms, due to the fact that stuff that was supposed to go to the
> > : screen is redirected to a file instead.  Some programs sense that and
> > : behave differently, e.g. with binary non-printable characters or with
> > : special character sequences.  By redirecting the output to files, then
> > : displaying it on the screen, the output may look strangely, or have
> > : some more grave effect, like terminating output prematurely.
> > 
> > I don't see why it would terminate prematurely
> 
> It was long ago, but I presume I thought about the ^Z character that
> some programs write or interpret to signal end of file.  Writing that
> to the console stops output, AFAIR.

Oh, that's still done? When I last used Dos in the 1990s it was
already obsolete and few programs did it. OK, so you'll have to
strip them. Fortunately, that's not too hard, I assume you have
already implemented it.

> > What I'm saying is the circumstances where this is a real problem
> > seem rather exotic, and noone's forcing you to use the option. If we
> > were talking about changing make's default behaviour, the concern
> > may be justified, but we aren't.
> 
> I just mentioned some caveats that people might find surprising,
> that's all.  Perhaps those caveats should be mentioned in the
> documentation.  No other intentions.

OK, we can add a few sentences in the docs. Have you already written
something (just to avoid further dupliation of work)?

> > - Factor out is_same_file() into a function.
> 
> I already wrote this in my branch, please wait until I commit.
> 
> > - Turn file_ok() and fd_not_empty() into functions instead of
> >   macros. (No real need for macros, and functions may be easier to
> >   port.)
> 
> Fixed that as well here.

OK, so Paul now has two versions to choose from. :-)

Really, I just tried to be helpful. Since you were constantly
complaining about the design, I did the (small) things I saw to
improve about it. Other than that I didn't (and still don't) know
what's so bad about the design.

> There's one issue that perhaps needs discussing.  A mutex is
> identified by a handle, which on Windows is actually a pointer to an
> opaque object (maintained by the kernel).

And the pointer is the same between different processes?

> As such, using just 'int'
> for sync_handle is not wide enough, certainly not in 64-bit builds.
> Is it OK to use intptr_t instead?  Doing this cleanly might require to
> have a macro (I called it FD_FROM_HANDLE) to extract a file descriptor
> that is passed to a Posix fcntl; on Windows that macro will be a
> no-op, and on Posix platforms it's a simple cast.
> 
> Is this OK?

I can't follow you here. On POSIX, we don't need to pass a fd
because it's always stdout/stderr. Or do mean something else?

> > +/* Test whether a file contains any data. */
> > +static int
> > +fd_not_empty (int fd)
> > +{
> > +  return fd >= 0 && lseek (fd, 0, SEEK_END) > 0;
> > +}
> 
> Isn't this unnecessarily expensive (with large output volumes)?  Why
> not use fstat?

On POSIX both lseek and fstat are not very expensive. On Windows,
you said fstat was very expensive, didn't you? Or is lseek even
worse?

> > How about we introduce functions called acquire_semaphore() and
> > release_semaphore()? Oh, wait, we did.
> 
> Sorry, but this is not funny.  There are, indeed, such functions, but
> that's about all there is about modularity in the implementation.  The
> rest is code scattered all over the place, that _knows_ all too well
> that it is dealing with file descriptors, not with some abstract
> semaphores or mutexes.  Here's a good example:
>
> [...]
>
> There are 2 separate things intermixed here: determination of
> combined_output and determination of the resource to use as a mutex.
> They are mixed because they both deal with file descriptors, and it
> was just convenient, by sheer luck (or maybe because of something
> else) to save a few if's and do them together.

I talked about this yesterday or so. I still don't see why you can't
keep this code as it. You can use sync_handle as a flag to make sure
the code is executed only once and ignore it for the locking
purpose.

Of course, you can rewrite the code to separate the two things, but
I still don't see that it's necessary. The checks to be done
(file_ok, is_same_file) are the same anyway, so I don't see it as a
design flaw.

Paul Smith wrote:

> > > +/* Test whether a file contains any data. */
> > > +static int
> > > +fd_not_empty (int fd)
> > > +{
> > > +  return fd >= 0 && lseek (fd, 0, SEEK_END) > 0;
> > > +}
> > 
> > Isn't this unnecessarily expensive (with large output volumes)?  Why
> > not use fstat?
> 
> This lseek() doesn't actually move the file reference: SEEK_END plus an
> offset of 0 is a no-op so it doesn't matter how large the file is.  This
> is just seeing if the position has moved since we opened the file (still
> at 0 or not); it just returns the current position in the file, which is
> known to the system directly without having to go ask anyone (it has to
> be so, since each file descriptor has its own position).

That's true about SEEK_CUR which was there originally. I actually
changed it to SEEK_END, which does move the position to the end. But
it doesn't matter since pump_from_tmp_fd() seeks back to the
beginning before reading. Also, I don't know that seeking to the back and
front of a file is expensive, even if the file is large. Nothing is actually
read by lseek (and even if it were, it would only need to look at the first
and last part of the file, not read all the content, if that was the worry).