[PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[Andrew Cooper]

These log lines are all in response to single system calls, and do not provide
any information which the immediate caller can't determine themselves.  It is
however exceedinly rude to put junk like this onto stderr, especially as
system call failures are not even error conditions in certain circumstances.

The FreeBSD logging has stale function names in, and solaris shouldn't have
passed code review to start with.

No functional change.

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>

The errno constructs in osdep_xenforeignmemory_map_resource() are addressed in
the following patch, to avoid adding complexity to this one.

This reduces the quantity of noise from unit tests, where certain syscall
failures are definitely not errors.
---
 tools/libs/foreignmemory/freebsd.c | 7 ++-----
 tools/libs/foreignmemory/linux.c   | 6 +-----
 tools/libs/foreignmemory/netbsd.c  | 2 --
 tools/libs/foreignmemory/solaris.c | 2 +-
 4 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/libs/foreignmemory/freebsd.c b/tools/libs/foreignmemory/freebsd.c
index 9a2796f0b7..04bfa806b0 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -65,10 +65,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 
     addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
     if ( addr == MAP_FAILED )
-    {
-        PERROR("xc_map_foreign_bulk: mmap failed");
         return NULL;
-    }
 
     ioctlx.num = num;
     ioctlx.dom = dom;
@@ -80,7 +77,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     if ( rc < 0 )
     {
         int saved_errno = errno;
-        PERROR("xc_map_foreign_bulk: ioctl failed");
+
         (void)munmap(addr, num << PAGE_SHIFT);
         errno = saved_errno;
         return NULL;
@@ -137,7 +134,7 @@ int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
         int saved_errno;
 
         if ( errno != ENOSYS )
-            PERROR("mmap resource ioctl failed");
+            ;
         else
             errno = EOPNOTSUPP;
 
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index d0eead1196..050b9ed3a5 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -171,10 +171,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED,
                 fd, 0);
     if ( addr == MAP_FAILED )
-    {
-        PERROR("mmap failed");
         return NULL;
-    }
 
     ioctlx.num = num;
     ioctlx.dom = dom;
@@ -273,7 +270,6 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
     {
         int saved_errno = errno;
 
-        PERROR("ioctl failed");
         (void)munmap(addr, num << PAGE_SHIFT);
         errno = saved_errno;
         return NULL;
@@ -330,7 +326,7 @@ int osdep_xenforeignmemory_map_resource(
         int saved_errno;
 
         if ( errno != fmem->unimpl_errno && errno != EOPNOTSUPP )
-            PERROR("ioctl failed");
+            ;
         else
             errno = EOPNOTSUPP;
 
diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
index 4ae60aafdd..565682e064 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -147,8 +147,6 @@ int osdep_xenforeignmemory_map_resource(
     rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
     if ( rc )
     {
-        PERROR("ioctl failed");
-
         if ( fres->addr )
         {
             int saved_errno = errno;
diff --git a/tools/libs/foreignmemory/solaris.c b/tools/libs/foreignmemory/solaris.c
index ee8aae4fbd..958fb01f6d 100644
--- a/tools/libs/foreignmemory/solaris.c
+++ b/tools/libs/foreignmemory/solaris.c
@@ -83,7 +83,7 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
     if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx) < 0 )
     {
         int saved_errno = errno;
-        PERROR("XXXXXXXX");
+
         (void)munmap(addr, num*XC_PAGE_SIZE);
         errno = saved_errno;
         return NULL;
-- 
2.11.0

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

[Andrew Cooper]

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(-)

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;
-- 
2.11.0

Re: [PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging"):
> These log lines are all in response to single system calls, and do not provide
> any information which the immediate caller can't determine themselves.  It is
> however exceedinly rude to put junk like this onto stderr, especially as
> system call failures are not even error conditions in certain circumstances.
> 
> The FreeBSD logging has stale function names in, and solaris shouldn't have
> passed code review to start with.
> 
> No functional change.

Thanks.

Reviewed-by: Ian Jackson <iwj@xenproject.org>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

>          int saved_errno = errno;
> -        PERROR("XXXXXXXX");
> +

That's particularly wtf...

Ian.

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

[Ian Jackson]

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.

Re: [PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging

[Andrew Cooper]

On 03/02/2021 17:16, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.15 1/2] libs/foreignmem: Drop useless and/or misleading logging"):
>> These log lines are all in response to single system calls, and do not provide
>> any information which the immediate caller can't determine themselves.  It is
>> however exceedinly rude to put junk like this onto stderr, especially as
>> system call failures are not even error conditions in certain circumstances.
>>
>> The FreeBSD logging has stale function names in, and solaris shouldn't have
>> passed code review to start with.
>>
>> No functional change.
> Thanks.
>
> Reviewed-by: Ian Jackson <iwj@xenproject.org>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks,

>
>>          int saved_errno = errno;
>> -        PERROR("XXXXXXXX");
>> +
> That's particularly wtf...

My thoughts exactly.

~Andrew

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

[Andrew Cooper]

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

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

[Ian Jackson]

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.

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

[Andrew Cooper]

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