Question regarding to Error Handling

[rmohr]

I see that in the rpc package, there is a private libvirtError struct, which contains the Code of the error. I would want to access it, to distinguish connection problems from errors like "domain does not exist". I find it in combination with helper functions like e.g. IsNotFound(err error) bool pretty helpful.

Are there some ideas floating around on how proper error handling is done? Would it make sense to make LibvirtError public and add the error code constants to the project?

[trapgate]

You ask a good question.

I don't think making the libvirtError struct public is the right thing, because much of what it contains isn't going to be useful to a client of the library. Even when it comes to the error codes, most of the time clients just want to know whether a call succeeded or failed. But there may be exceptions.

I'd suggest a variation on what your PR contains: keep the libvirtError struct private. With the the Error() func you added, decodeError now returns libvirtErrors, but they're opaque errors to clients of the library. Then you can add the helper functions that reason about them to go-libvirt itself, and they look would like this:

func ErrIsNotFound(e error) bool {
    err, ok := e.(libvirtError)
    if !ok {
        return false
    }
    return err.Code == ErrNotFound
}

A more complete example to show what I mean: https://play.golang.org/p/o2YyZpPoSfO

One criticism you could make of the approach I'm suggesting is that it potentially forces lots of clients of go-libvirt to add their own helper methods to the library, in the name of keeping some error codes private. But if what you're looking for is helpers that classify a range of errors, like all the connection errors, then it may be that a small number of helpers is all that will ever be needed.

[rmohr]

Then you can add the helper functions that reason about them to go-libvirt itself, and they look would like this:

How can I convert the error to the private type outside of its package? the PR already returns just the interface error.

Would having a libvirt.Error which looks like this:

type Error struct {
  Message string
  Code ErrorCode
  Domain ErrorCode 
}

func (e Error) Error() string {
  return e.Message
}

be acceptable?

One criticism you could make of the approach I'm suggesting is that it potentially forces lots of clients of go-libvirt to add their own helper methods to the library, in the name of keeping some error codes private.

Letting people write their own error helpers sounds good to me. What you suggest is exactly what I had in mind.

[trapgate]

How can I convert the error to the private type outside of its package? the PR already returns just the interface error.

Right, you can't do it outside of the go-libvirt package, because the libvirtError struct is private. But you can call the ErrIsNotFound function above, passing in the opaque error. ErrIsNotFound has to be part of go-libvirt, not the client.

To be clearer about what I mean, this is what I'm suggesting:

To clients of go-libvirt, any errors returned are opaque errors - opaque meaning "some type that implements the error interface". Since libvirtErrors would now implement the Error() interface, they can now be returned as opaque errors too. If client code needs to classify the error, they would call a helper function like ErrIsNotFound above. These helper functions would need to be inside go-libvirt itself, because they attempt to cast the opaque error back to a private error type. I'm also suggesting that the error codes should remain private to go-libvirt, rather than becoming part of its public API.

[rmohr]

I'm also suggesting that the error codes should remain private to go-libvirt, rather than becoming part of its public API.

I agree that for many people the opaque error is good enough, and having helper functions inside go-libvirt makes sense. Also the IsNotFound would mostly meet meet my requirement.

But to be honest, not having a public error does not sound very appealing for me. As far as I know, also in the go core packages pretty much all errors are public. Examples are os.PathError, os.LinkError, os.SyscallError. Still there exist helper functions for the most common use-cases like os.IsNotExist where the interface looks like like func IsNotExist(err error) bool, which looks like your signature suggestion. There we have the same situation like with the huge amount of libvirt error codes. There exist a lot of errno error codes which might be interesting for some users, but most are already happy with the predefined helper functions.

[trapgate]

It's true that some of the modules in the standard library have public error types, but they're not very common - I don't think there's any module in the standard library whose functions all return a special error type. The vast majority of the standard library functions return simple errors.

There are exceptions though, where the caller might need more information about the error. The ones you mention from the os package all have extra strings, so they're mostly used for building better error messages for users, but there are other examples (encoding/json.MarshalerError, for example) which have other go types. What I've never seen though are error types containing error codes, either errno values or enums from another application like libvirt. You certainly could do that, but I think there are two decent reasons not to: 1) it adds a bunch of consts to the library API, of which probably 99% will never be used, and 2) client code ends up being very non-idiomatic go.

I think there are two ways to do this don't have those problems. We could define some helper functions as I was suggesting before; or we could define some public errors, like ConnectionError; NoDomainError; etc, and return those in the right circumstances.

I don't have a problem with the second solution, actually. I just don't think the libvirt error codes should be part of the public API. This is a little different from what you were suggesting above, in that go-libvirt wouldn't have a special error type returned from all calls. Instead it would return one of the public error types when the error meets certain conditions (is a connection error, or is a domain-not-found error, etc.) I don't know that any of those error types would need any additional fields other than message, but there might be exceptions. I wouldn't put error codes in them.

Then there's the question of how many error types we'd need. I don't think the number would be very large, and I wouldn't add new ones unless there was a good reason.

[rmohr]

What I've never seen though are error types containing error codes, either errno values or enums from another application like libvirt.

Could it be that you mix up returning an error interface and the actual struct? See for instance https://golang.org/pkg/syscall/#Errno. It is public and it implements the error interface but you can also cast it to the actual errno value itoa(int(e)) if needed. While the standard libs always return the error interface, all error implementations in the standard lib I know are public types.

[rmohr]

Closed #57 for now since I don't need this library right now. Anyway I am sorry that this did not work out. Great work you guys do here 👍