| Message ID | 20210203163750.7564-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.15,1/2] libs/foreignmem: Drop useless and/or misleading logging | expand |
Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource"):
> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
> support.
>
> The Linux logic was contorted in what appears to be a deliberate attempt to
> skip the now-deleted logic for the EOPNOTSUPP case. Simplify it.
AFAICT this is a mixture of cleanup/refactoring, and bugfix. Is that
correct ?
> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> index 565682e064..c0b1b8f79d 100644
> --- a/tools/libs/foreignmemory/netbsd.c
> +++ b/tools/libs/foreignmemory/netbsd.c
> @@ -147,6 +147,9 @@ int osdep_xenforeignmemory_map_resource(
> rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
> if ( rc )
> {
> + if ( errno == ENOSYS )
> + errno = EOPNOTSUPP;
> +
> if ( fres->addr )
> {
> int saved_errno = errno;
Specifically, I guess this is the bugfix ?
Ian.
On 03/02/2021 17:18, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource"): >> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI >> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel >> support. >> >> The Linux logic was contorted in what appears to be a deliberate attempt to >> skip the now-deleted logic for the EOPNOTSUPP case. Simplify it. > AFAICT this is a mixture of cleanup/refactoring, and bugfix. Is that > correct ? > >> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c >> index 565682e064..c0b1b8f79d 100644 >> --- a/tools/libs/foreignmemory/netbsd.c >> +++ b/tools/libs/foreignmemory/netbsd.c >> @@ -147,6 +147,9 @@ int osdep_xenforeignmemory_map_resource( >> rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr); >> if ( rc ) >> { >> + if ( errno == ENOSYS ) >> + errno = EOPNOTSUPP; >> + >> if ( fres->addr ) >> { >> int saved_errno = errno; > Specifically, I guess this is the bugfix ? It is a bugfix on NetBSD (for brand new functionality), and cleanup on FreeBSD/Linux specifically split out of the previous patch. ~Andrew
Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource"):
> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI
> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel
> support.
>
> The Linux logic was contorted in what appears to be a deliberate attempt to
> skip the now-deleted logic for the EOPNOTSUPP case. Simplify it.
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Sorry for my earlier confusion. I had lost the context between the
two patches. I will explain my reasoning for the R-A:
For the first two hunks (freebsd.c): these are consequential cleanup
from patch 1/2 of this series. Splitting this up made this easier to
review and we don't want to leave the rather unfortunate constructs
which arise from some hunks of 1/1. IOW, the combination of 1/1 plus
the first two hunks here is definitely release-worthy and the split
has helped review.
The final hunk is a straightforward bugfix.
This combination of two completely different kinds of thing is a bit
confusing but now that I have explained it to myself I'm satisfied.
Ian.
On 03/02/2021 17:51, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH for-4.15 2/2] libs/foreignmem: Fix/simplify errno handling for map_resource"): >> Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI >> appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel >> support. >> >> The Linux logic was contorted in what appears to be a deliberate attempt to >> skip the now-deleted logic for the EOPNOTSUPP case. Simplify it. > Release-Acked-by: Ian Jackson <iwj@xenproject.org> > > Sorry for my earlier confusion. I had lost the context between the > two patches. I will explain my reasoning for the R-A: > > For the first two hunks (freebsd.c): these are consequential cleanup FreeBSD and Linux. > from patch 1/2 of this series. Splitting this up made this easier to > review and we don't want to leave the rather unfortunate constructs > which arise from some hunks of 1/1. IOW, the combination of 1/1 plus > the first two hunks here is definitely release-worthy and the split > has helped review. > > The final hunk is a straightforward bugfix. > > This combination of two completely different kinds of thing is a bit > confusing but now that I have explained it to myself I'm satisfied. Thanks, ~Andrew
diff --git a/tools/libs/foreignmemory/freebsd.c b/tools/libs/foreignmemory/freebsd.c index 04bfa806b0..d94ea07862 100644 --- a/tools/libs/foreignmemory/freebsd.c +++ b/tools/libs/foreignmemory/freebsd.c @@ -133,9 +133,7 @@ int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem, { int saved_errno; - if ( errno != ENOSYS ) - ; - else + if ( errno == ENOSYS ) errno = EOPNOTSUPP; if ( fres->addr ) diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c index 050b9ed3a5..c1f35e2db7 100644 --- a/tools/libs/foreignmemory/linux.c +++ b/tools/libs/foreignmemory/linux.c @@ -325,9 +325,7 @@ int osdep_xenforeignmemory_map_resource( { int saved_errno; - if ( errno != fmem->unimpl_errno && errno != EOPNOTSUPP ) - ; - else + if ( errno == fmem->unimpl_errno ) errno = EOPNOTSUPP; if ( fres->addr ) diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c index 565682e064..c0b1b8f79d 100644 --- a/tools/libs/foreignmemory/netbsd.c +++ b/tools/libs/foreignmemory/netbsd.c @@ -147,6 +147,9 @@ int osdep_xenforeignmemory_map_resource( rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr); if ( rc ) { + if ( errno == ENOSYS ) + errno = EOPNOTSUPP; + if ( fres->addr ) { int saved_errno = errno;
Simplify the FreeBSD logic, and duplicate it for NetBSD as the userpace ABI appears to be to consistently provide EOPNOTSUPP for missing Xen/Kernel support. The Linux logic was contorted in what appears to be a deliberate attempt to skip the now-deleted logic for the EOPNOTSUPP case. Simplify it. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Jackson <iwj@xenproject.org> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Manuel Bouyer <bouyer@netbsd.org> --- tools/libs/foreignmemory/freebsd.c | 4 +--- tools/libs/foreignmemory/linux.c | 4 +--- tools/libs/foreignmemory/netbsd.c | 3 +++ 3 files changed, 5 insertions(+), 6 deletions(-)