Re: [PATCH] Support regular file type in _swistat for ARM

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 04/06/2020 10:53, Joe Ramsay wrote:
> Hi!
> 
> Previously, the implementation of _swistat() for ARM assumed all files
> to be of type character device. This change adds support for stat-ing
> regular files. Tested with arm-none-eabi

Firstly, there's a second version of this function, in
newlib/sysdeps/arm; the two really need to be kept in sync.
Unfortunately, there have been other cases where this hasn't happened,
so things are getting a bit messy now....


> ---
> libgloss/arm/syscalls.c | 43 +++++++++++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/libgloss/arm/syscalls.c b/libgloss/arm/syscalls.c
> index fc394f9..b4bd329 100644
> --- a/libgloss/arm/syscalls.c
> +++ b/libgloss/arm/syscalls.c
> @@ -728,33 +728,48 @@ _sbrk (ptrdiff_t incr)
> int
>  _swistat (int fd, struct stat * st)
> {
> -  struct fdent *pfd;
> -  int res;
> -
> -  pfd = findslot (fd);
> +  struct fdent *pfd = findslot (fd);
>    if (pfd == NULL)
>      {
>        errno = EBADF;
>        return -1;
>      }
> 
> -  /* Always assume a character device,
> -     with 1024 byte blocks. */
> -  st->st_mode |= S_IFCHR;
> -  st->st_blksize = 1024;
> +  int isCharDevice, flen;
> #ifdef ARM_RDI_MONITOR
> -  res = checkerror (do_AngelSWI (AngelSWI_Reason_FLen, &pfd->handle));
> +  isCharDevice = checkerror (do_AngelSWI (AngelSWI_Reason_IsTTY, &pfd->handle));
> +  flen = checkerror (do_AngelSWI (AngelSWI_Reason_FLen, &pfd->handle));
> #else
>    asm ("mov r0, %2; swi %a1; mov %0, r0"
> -       : "=r" (res)
> +       : "=r" (isCharDevice)
> +       : "i" (SWI_IsTTY), "r" (pfd->handle)
> +       : "r0");
> +  checkerror(isCharDevice);
> +  asm ("mov r0, %2; swi %a1; mov %0, r0"
> +       : "=r" (flen)
>         : "i" (SWI_Flen), "r" (pfd->handle)
>         : "r0");
> -  checkerror (res);
> +  checkerror(flen);
> #endif

I don't think you should make the second SWI call if the first one
failed, so you should bail out immediately on failure.  Also, checkerror
already calls error, so it is redundant to call it a second time.

> -  if (res == -1)
> +
> +  if (isCharDevice != 0 && isCharDevice != 1)
> +    return error(-1);

I'm not sure the logic of that is right, even though in practice the
only possible results are 0, 1 and -1.  error() should only be called if
isCharDevice is -1, since otherwise the errno will be set to something
meaningless, but see above, this needs to be done earlier.

> +
> +  if (flen == -1)
>      return -1;
> -  /* Return the file size. */
> -  st->st_size = res;
> +
> +  st->st_size = flen;
> +  /* Always assume 1024 byte blocks */
> +  st->st_blksize = 1024;
> +
> +  if (isCharDevice)
> +    st->st_mode |= S_IFCHR;
> +  else
> +    {
> +      st->st_mode |= S_IFREG;
> +      st->st_blocks = (flen + 1023) / 1024;
> +    }

Try to avoid manifest constants in the code.  Use a #define and write
expressions in terms of that.


R.

> +
>    return 0;
> }
> 
> --
> 2.7.4
>