Re: [PATCH v2] Associate target_ops with target_fileio file descriptors

[Pedro Alves <palves@redhat.com>]

On 03/24/2015 04:17 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 03/20/2015 04:06 PM, Gary Benson wrote:
>>> +
>>> +  /* Search for closed handles to reuse.  */
>>> +  if (lowest_closed_fd >= 0)
>>> +    {
>>> +      for (index = lowest_closed_fd;
>>> +	   VEC_iterate (fileio_fh_t, fileio_fhandles, index, fh);
>>> +	   index++)
>>> +	if (is_closed_fileio_fh (fh->fd))
>>> +	  break;
>>
>> You don't really need 'index'.  If this walks using
>> lowest_closed_fd instead, like:
>>
>>       for (;
>> 	   VEC_iterate (fileio_fh_t, fileio_fhandles,
>>                         lowest_closed_fd, fh);
>> 	   lowest_closed_fd++)
>> 	if (is_closed_fileio_fh (fh->fd))
>> 	  break;
>>
>> Then the next time around we skip slots we already know are
>> open.
>>
>>> +    }
>>> +
>>> +  /* Push a new handle if no closed handles were found.  */
>>> +  if (index == total)
>>> +    {
>>> +      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
>>> +      lowest_closed_fd = -1;
>>> +    }
>>> +
>>> +  /* Fill in the handle and return its index.  */
>>> +  fh->t = t;
>>> +  fh->fd = fd;
>>> +
>>> +  return index;
>>> +}
> 
> You do need another variable though.  The function returns the
> index of the file handle it just used, so you need to save that
> across the bit that sets lowest_closed_fd to -1.  I rearranged
> things to this, but it's longer and I'd argue harder to follow:

We can get the index with VEC_address.  And if we get rid
of the -1 state, things get event simpler, and release_fileio_fd can
be simplified too, like this (untested):

/* Index into fileio_fhandles of the lowest handle that might be
   closed.  This permits handle reuse without searching the whole list
   each time a new file is opened.  */
static int lowest_closed_fd = 0;

static int
acquire_fileio_fd (struct target_ops *t, int fd)
{
  fileio_fh_t *fh, buf;

  gdb_assert (!is_closed_fileio_fh (fd));

  /* Search for closed handles to reuse.  */
  for (;
       VEC_iterate (fileio_fh_t, fileio_fhandles,
		    lowest_closed_fd, fh);
       lowest_closed_fd++)
    if (is_closed_fileio_fh (fh->fd))
      break;

  /* Push a new handle if no closed handles were found.  */
  if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
    {
      fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
      lowest_closed_fd++;
    }

  /* Fill in the handle.  */
  fh->t = t;
  fh->fd = fd;

  /* Return its index.  */
  return fh - VEC_address (fileio_fh_t, fileio_fhandles);
}

static void
release_fileio_fd (int fd, fileio_fh_t *fh)
{
  fh->fd = -1;
  lowest_closed_fd = min (lowest_closed_fd, fd);
}

Thanks,
Pedro Alves