This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Introduce new shared function remote_fileio_to_fio_error
- From: Pedro Alves <palves at redhat dot com>
- To: Gary Benson <gbenson at redhat dot com>, gdb-patches at sourceware dot org
- Date: Thu, 09 Apr 2015 11:17:52 +0100
- Subject: Re: [PATCH] Introduce new shared function remote_fileio_to_fio_error
- Authentication-results: sourceware.org; auth=none
- References: <1428573072-8639-1-git-send-email-gbenson at redhat dot com>
On 04/09/2015 10:51 AM, Gary Benson wrote:
> Hi all,
>
> This commit introduces a new shared function to replace three
> identical functions in various places in the codebase.
>
> Ok to commit?
>
This is OK, but I think the "remote" in the name is really wrong.
This is converting a _host_ errno to a fileio error. I think the
function name should reflect that, and not mention remote at all.
This is a preexisting issue with remote_fileio_to_fio_stat as well
(should be called something like host_stat_to_fio_stat / stat_to_fio_stat /
stat_to_fileio_stat / fileio_host_stat_to_fio_stat), so I'm not
pushing back on this new case. We should probably rename
the "common-remote-fileio.*" files too.
> gdb/gdbserver/ChangeLog:
>
> * hostio-errno.c (errno_to_fileio_error): Remove function.
E.g., the old name here was clear.
> Update caller to use remote_fileio_to_fio_error.
> +int
> +remote_fileio_to_fio_error (int error)
> +{
> + switch (error)
> + {
> + case EPERM:
> + return FILEIO_EPERM;
... otherwise this is truly confusing. Reading the function
name as "X to Y", makes one go "hmm, what has 'remote fileio' got
to do with about EPERM?' ?
> /* Convert a host-format mode_t into a bitmask of File-I/O flags. */
>
> static LONGEST
> diff --git a/gdb/inf-child.c b/gdb/inf-child.c
> index b7161ab..5e5763b 100644
> --- a/gdb/inf-child.c
> +++ b/gdb/inf-child.c
> @@ -311,7 +260,7 @@ inf_child_fileio_open (struct target_ops *self,
> the standard values. */
> fd = gdb_open_cloexec (filename, nat_flags, mode);
> if (fd == -1)
> - *target_errno = inf_child_errno_to_fileio_error (errno);
> + *target_errno = remote_fileio_to_fio_error (errno);
>
... and inf-child calling a "remote_"-named function is truly bizarre.
Thanks,
Pedro Alves