[6/9] golang/xenlight: Errors are negative
diff mbox series

Message ID 20191227163224.4113837-6-george.dunlap@citrix.com
State New
Headers show
Series
  • [1/9] golang/xenlight: Don't try to marshall zero-length arrays
Related show

Commit Message

George Dunlap Dec. 27, 2019, 4:32 p.m. UTC
Commit 871e51d2d4 changed the sign on the xenlight error types (making
the values negative, same as the C-generated constants), but failed to
flip the sign in the Error() string function.  The result is that
ErrorNonspecific.String() prints "libxl error: 1" rather than the
human-readable error message.

Get the error message index by inverting the error number once.

Also, always print the actual error value, rather than the inverted
value, for clarity.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/xenlight.go | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Nick Rosbrook Jan. 4, 2020, 7:27 p.m. UTC | #1
On Fri, Dec 27, 2019 at 11:33 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> Commit 871e51d2d4 changed the sign on the xenlight error types (making
> the values negative, same as the C-generated constants), but failed to
> flip the sign in the Error() string function.  The result is that
> ErrorNonspecific.String() prints "libxl error: 1" rather than the
> human-readable error message.

Since we're here, what would you think about re-defining libxlErrors
as a map[Error]string? That way, Error() can just be:

func (e Error) Error() string {
        if s, ok := libxlErrors[e]; ok {
                return s
        }

        return fmt.Sprintf("libxl error: %d", e)
}

I think it's less error-prone and easier to read. Thoughts?

-NR
George Dunlap Jan. 16, 2020, 4:59 p.m. UTC | #2
On 1/4/20 7:27 PM, Nick Rosbrook wrote:
> On Fri, Dec 27, 2019 at 11:33 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> Commit 871e51d2d4 changed the sign on the xenlight error types (making
>> the values negative, same as the C-generated constants), but failed to
>> flip the sign in the Error() string function.  The result is that
>> ErrorNonspecific.String() prints "libxl error: 1" rather than the
>> human-readable error message.
> 
> Since we're here, what would you think about re-defining libxlErrors
> as a map[Error]string? That way, Error() can just be:
> 
> func (e Error) Error() string {
>         if s, ok := libxlErrors[e]; ok {
>                 return s
>         }
> 
>         return fmt.Sprintf("libxl error: %d", e)
> }
> 
> I think it's less error-prone and easier to read. Thoughts?

Yes, that sounds better.  Done.

 -George

Patch
diff mbox series

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 99de68320b..c80f622e6b 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -65,13 +65,14 @@  var libxlErrors = [...]string{
 }
 
 func (e Error) Error() string {
-	if 0 < int(e) && int(e) < len(libxlErrors) {
-		s := libxlErrors[e]
+	eidx := -int(e)
+	if 0 < eidx && eidx < len(libxlErrors) {
+		s := libxlErrors[eidx]
 		if s != "" {
 			return s
 		}
 	}
-	return fmt.Sprintf("libxl error: %d", -e)
+	return fmt.Sprintf("libxl error: %d", e)
 }
 
 // Context represents a libxl_ctx.