diff mbox series

[for-4.15,2/2] libs/foreignmem: Fix/simplify errno handling for map_resource

Message ID 20210203163750.7564-2-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series [for-4.15,1/2] libs/foreignmem: Drop useless and/or misleading logging | expand

Commit Message

Andrew Cooper Feb. 3, 2021, 4:37 p.m. UTC
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(-)

Comments

Ian Jackson Feb. 3, 2021, 5:18 p.m. UTC | #1
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.
Andrew Cooper Feb. 3, 2021, 5:27 p.m. UTC | #2
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
Ian Jackson Feb. 3, 2021, 5:51 p.m. UTC | #3
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.
Andrew Cooper Feb. 3, 2021, 5:52 p.m. UTC | #4
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 mbox series

Patch

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;