bug-cvs
[Top][All Lists]
Advanced

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

Re: CVS problem with ssh


From: Derek Price
Subject: Re: CVS problem with ssh
Date: Wed, 13 Jul 2005 13:10:38 -0400
User-agent: Mozilla Thunderbird 1.0.2 (Windows/20050317)

Hi bug-gnulib,

I'd like to reopen a discussion on this list from last September: 
<http://lists.gnu.org/archive/html/bug-gnulib/2004-09/msg00010.html>.

At the end of that discussion I didn't create the blocking-io module due
mostly to Paul Eggert's objections (and a lack of time on my part,
especially for a solution which sounded like it would be rejected). 
Anyhow, RMS wanted the problem this fixes in CVS fixed badly enough that
he wrote most of the stdio wrappers we discussed himself.  The public
portion of that new discussion is archived here:
<http://lists.gnu.org/archive/html/bug-cvs/2005-07/msg00017.html>.

In any case, after a few iterations and testing in CVS on GNU/Linux, RMS
& I came up with the two files I've attached to this email.  I believe
they answer all of the objections raised in the first thread linked
above except Bruno's comment on porting to WOE32 and the following
comments, which I will attempt to address here.  The WOE32 issue and
conversion to a proper GNULIB module can wait until the disposition of
this code is settled.

Paul Eggert wrote:

>You have to include stdio.h first, then #undef fwrite, then
>#define fwrite.  It can be done (Emacs did it, after all), but I don't
>recommend it.
>

Isn't this the standard behavior for a GNULIB module's header file? 
This is almost exactly what unlocked-io does, after all.  In any case, I
think this is now addressed by the functionality in blocking-io.h.

>/ POSIX says fflush sets errno.  I'm worried about implementations that/
>/ don't conform (or perhaps conform to an older standard?)./


In the cases where fflush does not set errno, we are no worse off than
we were before.  The way the new wrappers are written, the function
behaviors should remain unchanged if errno != EAGAIN (or EWOULDBLOCK on
BSD) and when when they do not return short writes.

>As far as I can tell POSIX doesn't guarantee that repeated fflushes
>will eventually write all the data, with no duplicates, in this
>situation.  Here's the spec:
>http://www.opengroup.org/onlinepubs/009695399/functions/fflush.html
>

Maybe not, but isn't this by far the implication of EAGAIN in most other
contexts?  Couldn't do what you asked this time, try *AGAIN*?  I can
understand the problem with fwrite and an object size > 1 since there is
no way for the function to return a partial item count, but once stdio
has the data, why should fflush not track what was written properly? 
Aside from bugs, this is what I would expect from most fflush
implementations.  How about we install the module and see what sort of
problem reports we get?

Bruno Habile wrote:

>Larry Jones wrote:
>>/ they should be testing for writability with select before trying to/
>>/ write to stderr rather than just blindly putting stderr into nonblocking/
>>/ mode/
>
>Agreed: On platforms where select() or poll() is available, there is no
>reason to use the old non-blocking I/O facility.
>

With this fix, CVS and potentially other tools will work with the broken
SSH implementations and other, hypothetical, future tools, which are as
rude in their treatment of shared file descriptors, regardless of
whether the OpenSSH folks fix this issue.


I would like to see this installed in several locations for the
following reasons, all basically various aspects of the advantages of
code-sharing:

GLIBC:

   1. This module provides useful functionality often expected by
      programs: guaranteed blocking behavior from stdio.
   2. Would promote use in other tools, avoiding similar problems in the
      future.
   3. Would be ported to more platforms more quickly, decreasing
      liklihood of broken CVS releases.
   4. More eyes on this code would mean fewer bugs.


GNULIB:

   1. All the reasons I want this in GLIBC.
   2. Ease of importing GLIBC updates to this code into CVS.


CVS:

   1. This fix certainly shouldn't hurt anything, once the portability
      issues are dealt with.
   2. Will hopefully fix interaction with OpenSSH, and potentially other
      tools, on most platforms.


I might get away with installing this just in CVS, but I am afraid that
CVS doesn't have the manpower to deal with the potential porting issues
raised by various parties in the earlier discussions without the GLIBC
and GNULIB support, and RMS said he would back getting this into GLIBC &
GNULIB to get the fix into CVS.  Regardless, as I already stated, I
think this module provides useful functionality often expected by
programs: guaranteed blocking behavior from stdio.

Regards,

Derek
#ifndef BLOCKING_IO_H
# define BLOCKING_IO_H

/* Get va_list.  */
# include <stdarg.h>
# include <stdio.h>

# define printf blocking_printf
# define fprintf blocking_fprintf
# define vprintf blocking_vprintf
# define vfprintf blocking_vfprintf
# undef putchar
# define putchar blocking_putchar
# undef putc
# define putc blocking_putc
# define fputc blocking_putc
# define puts blocking_puts
# define fputs blocking_fputs
# define fwrite blocking_fwrite
# define fflush blocking_fflush
# define fclose blocking_fclose

int blocking_printf (const char *format, ...);
int blocking_fprintf (FILE *stream, const char *format, ...);
int blocking_vprintf (const char *format, va_list ap);
int blocking_vfprintf (FILE *stream, const char *format, va_list ap);
int blocking_putchar (char c);
int blocking_putc (char c, FILE *stream);
int blocking_puts (const char *s);
int blocking_fputs (const char *s, FILE *stream);
int blocking_fwrite (const void *buffer, size_t size, size_t count,
                     FILE *stream);
int blocking_fflush (FILE *stream);
int blocking_fclose (FILE *stream);
#endif /* BLOCKING_IO_H */
/* The idea of this module is to help programs cope with output
   streams that have been set nonblocking.

   To use it, simply make these definitions in your other files:

   #define printf blocking_printf
   #define fprintf blocking_fprintf
   #define vprintf blocking_vprintf
   #define vfprintf blocking_vfprintf
   #undef putchar
   #define putchar blocking_putchar
   #undef putc
   #define putc blocking_putc
   #define fputc blocking_putc
   #define puts blocking_puts
   #define fputs blocking_fputs
   #define fwrite blocking_fwrite
   #define fflush blocking_fflush

   and link with this module.  */

#include <errno.h>
#include <stdarg.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/select.h>


/* EWOULDBLOCK is not defined by POSIX, but some BSD systems will
   return it, rather than EAGAIN, for nonblocking writes.  */
#ifdef EWOULDBLOCK
# define blocking_error(err) ((err) == EWOULDBLOCK || (err) == EAGAIN)
#else
# define blocking_error(err) ((err) == EAGAIN)
#endif

int
blocking_fwrite (const void *buffer, size_t size, size_t count, FILE *stream)
{
  size_t count_left = count;
  size_t count_written = 0;
  int desc = fileno (stream);

  if (size == 0 || count == 0)
    return 0;

  /* Handle the case where SIZE * COUNT overflows.
     (The comparison is designed to be conservative.)  */
  if (size > 1 && (double)size * (double)count > SIZE_MAX / 4)
    {
      const char *p = (const char *) buffer;
      for (count_written = 0; count_written < count; count_written++)
        {
          if (blocking_fwrite (p, sizeof *p, size, stream) < size)
            break;
          p += size;
        }
      return count_written;
    }

  /* When SIZE is > 1, convert to the byte-wise case
     since that is the only way fwrite undertakes not to repeat
     writing part of the data.  */
  if (size > 1)
    {
      size_t total = size * count;
      unsigned int value = blocking_fwrite (buffer, sizeof *buffer, total,
                                            stream);
      /* It is ok, according to POSIX, if VALUE is not a multiple
         of SIZE, because fwrite in that case can write part of an object.  */
      return value / size;
    }

  while (1)
    {
      int written;
      fd_set set;

      written = fwrite (buffer, sizeof *buffer, count_left, stream);
      count_written += written;
      if (written == count_left)
        break;
      if (blocking_error (errno))
        break;
      
      /* Wait for space to be available.  */
      FD_ZERO (&set);
      FD_SET (desc, &set);
      select (desc + 1, NULL, &set, NULL, NULL);

      buffer += written;
      count_left -= written;
    }

  return count_written;
}

int
blocking_printf (const char *format, ...)
{
  int out_count;
  char *buffer;
  int size = 1000;

  while (1)
    {
      va_list argptr;

      buffer = malloc (size);
      if (!buffer)
        {
          /* Failed malloc on some non-posix systems (e.g. mingw) fail to set
             errno.  */
          if (!errno) errno = ENOMEM;
          return 0;
        }

      /* Format it into BUFFER, as much as fits.  */

      va_start (argptr, format);
      out_count = vsnprintf (buffer, size, format, argptr);
      va_end (argptr);
      /* OUT_COUNT now is number of bytes needed, not counting final null.  */

      if (out_count + 1 <= size)
        break;

      /* If it did not fit, try again with more space.  */

      free (buffer);
      size = out_count + 1;
    }

  /* Now we have the desired output in BUFFER.  Output it.  */

  blocking_fwrite (buffer, sizeof *buffer, out_count, stdout);

  free (buffer);
  return out_count;
}

int
blocking_fprintf (FILE *stream, const char *format, ...)
{
  int out_count;
  char *buffer;
  int size = 1000;

  while (1)
    {
      va_list argptr;

      buffer = malloc (size);
      if (!buffer)
        {
          /* Failed malloc on some non-posix systems (e.g. mingw) fail to set
             errno.  */
          if (!errno) errno = ENOMEM;
          return 0;
        }

      /* Format it into BUFFER, as much as fits.  */

      va_start (argptr, format);
      out_count = vsnprintf (buffer, size, format, argptr);
      va_end (argptr);
      /* OUT_COUNT now is number of bytes needed, not counting final null.  */

      if (out_count + 1 <= size)
        break;

      /* If it did not fit, try again with more space.  */

      free (buffer);
      size = out_count + 1;
    }

  /* Now we have the desired output in BUFFER.  Output it.  */

  blocking_fwrite (buffer, sizeof *buffer, out_count, stream);

  free (buffer);
  return out_count;
}

int
blocking_vprintf (const char *format, va_list ap)
{
  int out_count;
  char *buffer;
  int size = 1000;

  while (1)
    {
      va_list argptr;

      buffer = malloc (size);
      if (!buffer)
        {
          /* Failed malloc on some non-posix systems (e.g. mingw) fail to set
             errno.  */
          if (!errno) errno = ENOMEM;
          return 0;
        }

      /* Format it into BUFFER, as much as fits.  */

      va_copy (argptr, ap);
      out_count = vsnprintf (buffer, size, format, argptr);
      va_end (argptr);
      /* OUT_COUNT now is number of bytes needed, not counting final null.  */

      if (out_count + 1 <= size)
        break;

      /* If it did not fit, try again with more space.  */

      free (buffer);
      size = out_count + 1;
    }

  /* Now we have the desired output in BUFFER.  Output it.  */

  blocking_fwrite (buffer, sizeof *buffer, out_count, stdout);

  free (buffer);
  return out_count;
}

int
blocking_vfprintf (FILE *stream, const char *format, va_list ap)
{
  int out_count;
  char *buffer;
  int size = 1000;

  while (1)
    {
      va_list argptr;

      buffer = malloc (size);
      if (!buffer)
        {
          /* Failed malloc on some non-posix systems (e.g. mingw) fail to set
             errno.  */
          if (!errno) errno = ENOMEM;
          return 0;
        }

      /* Format it into BUFFER, as much as fits.  */

      va_copy (argptr, ap);
      out_count = vsnprintf (buffer, size, format, argptr);
      va_end (argptr);
      /* OUT_COUNT now is number of bytes needed, not counting final null.  */

      if (out_count + 1 <= size)
        break;

      /* If it did not fit, try again with more space.  */

      free (buffer);
      size = out_count + 1;
    }

  /* Now we have the desired output in BUFFER.  Output it.  */

  blocking_fwrite (buffer, sizeof *buffer, out_count, stream);

  free (buffer);
  return out_count;
}

int
blocking_putchar (char c)
{
  return (blocking_fwrite (&c, sizeof c, 1, stdout) < 1 ? EOF
          : (int) (unsigned char) c);
}

/* This serves for fputs also.  */
int
blocking_putc (char c, FILE *stream)
{
  return (blocking_fwrite (&c, sizeof c, 1, stream) < 1 ? EOF
          : (int) (unsigned char) c);
}

int
blocking_puts (const char *s)
{
  size_t len = strlen (s);
  return (blocking_fwrite (s, sizeof *s, len, stdout) < len ? EOF : 0);
}

int
blocking_fputs (const char *s, FILE *stream)
{
  size_t len = strlen (s);
  return (blocking_fwrite (s, sizeof *s, len, stream) < len ? EOF : 0);
}

int
blocking_fflush (FILE *stream)
{
  int desc = fileno (stream);

  while (1)
    {
      int value;
      fd_set set;

      value = fflush (stream);
      if (value == 0 || blocking_error (errno))
        return value;
      
      /* Wait for space to be available.  */
      FD_ZERO (&set);
      FD_SET (desc, &set);
      select (desc + 1, NULL, &set, NULL, NULL);
    }
}

int
blocking_fclose (FILE *stream)
{
  int desc = fileno (stream);

  while (1)
    {
      int value;
      fd_set set;

      value = fclose (stream);
      if (value == 0 || blocking_error (errno))
        return value;
      
      /* Wait for space to be available.  */
      FD_ZERO (&set);
      FD_SET (desc, &set);
      select (desc + 1, NULL, &set, NULL, NULL);
    }
}

reply via email to

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