On Fri, Feb 09, 2024 at 09:37:24AM +0000, Daniel P. Berrangé wrote:
> On Fri, Feb 09, 2024 at 08:43:56AM +0100, Alice Frosi wrote:
> > On Wed, Feb 7, 2024 at 11:13 AM Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > wrote:
> >
> > > On Wed, Feb 07, 2024 at 01:54:30AM -0800, Andrea Bolognani wrote:
> > > > On Wed, Feb 07, 2024 at 10:06:46AM +0100, Alice Frosi wrote:
> > > > > Hi everyone,
> > > > >
> > > > > I would like to bring up a possible issue with the
> > > libvirt-go-xml-module
> > > > > and request your advice.
> > > > >
> > > > > We are considering using libvirtxml [2] as an alternative of the custom
> > > > > types for the libvirt schema in KubeVirt [1].
> > > > > Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best
> > > > > guidelines. One thing I've noticed, though, is that libvirtml
> > > consistently
> > > > > uses uint or int rather than uint32 or int32. This is in opposition to
> > > the
> > > > > primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses
> > > them
> > > > > in the schema.go [4].
> > > >
> > > > I sure spotted several uses of (u)int in there :)
> > > >
> > > > > Would it be possible to align the libvirtxml go library to the
> > > Kubernetes
> > > > > conventions?
> > > >
> > > > Since this library is all about handling XML and doesn't talk at all
> > > > with the underlying C library, I guess it would be possible to
> > > > introduce explicit sizing for all struct members. I don't think we
> > > > can do that without breaking API though.
> > >
> > > It would also be a major effort to decide between int32 and int64
> > > if we want to honour the other k8s rule that says to avoid int64
> > > unless absolutely required.
> > >
> > > > Another topic entirely would be whether the same is possible for the
> > > > main binding. In that case, the underlying C library dictates the
> > > > types, so I'm not sure we could change them at the Go level even if
> > > > we wanted to. I know that you're not currently asking for this, I'm
> > > > just trying to consider the full picture.
> > >
> > > In the C API we have been pretty careful to use 'unsigned long long'
> > > everywhere it was needed, which is mapped to int64 in the go bindings.
> > >
> > > There were just 2 APIs where we mistakenly used 'unsigned long'
> > > which varied in size, but we map them to int64 in Go too.
> > >
> > > In any case, while k8s might care about 32-bit, in the virtualization
> > > space almost no one cares anymore. i386 is basically dead and liable
> > > to be removed from QEMU. Arm7 is limping along, but its use of virt
> > > is larged in embedded / special scenarios - no one is going to be
> > > offering public cloud with arm7.
> > >
> > > IOW, I see no reason for kubevirt to care about 32-bit targets, and
> > > thus for all practical purposes any use of 'int' can be assumed to
> > > always be 64-bit [1].
> >
> > But this isn't clear if we keep using (u)int types. What I'm suggesting
> > here is if it would be possible in a next version of the library to use
> > more explicit types.
> >
> > My concern is that KubeVirt uses (u)int32 as much as possible and in this
> > case, we might have a mismatch of the types.
>
> The issue is that this would be a change of API, and significant amount
> of work to do to audit each case, to solve a precision problem that
> doesn't appear to have any real world functional impact.
Yes, this would force us to declare a v2 API. Annoying, but less so
for a Go library than a C one. And let's ignore the amount of work
that would be necessary for a moment.
I tend to agree with Alice that this would be good to have.
Precisely-sized integer types are very commonly used in Go and Rust,
and they make the API a lot better.
Keep in mind that KubeVirt uses uint32 a lot for its public API (as
recommended by the Kubernetes guidelines) and then, internally, needs
to convert its structs to libvirtxml structs and back.
So suppose that the some VM attribute is represented as uint32 in the
KubeVirt API and as uint in the libvirtxml API. Going from the former
to the latter works fine, as uint is always going to be at least as
large as uint32; however, when going in the other direction (query
information from libvirt, unmarshal the XML using libvirtxml and then
copy the value over to KubeVirt's public struct) has to be done with
care.
Silly example to demonstrate the potential issue:
$ cat uint.go
package main
import (
"fmt"
"math"
)
func main() {
var k uint32 = math.MaxUint32
var l uint = uint(k)
fmt.Println("k = ", k)
fmt.Println("l = ", l)
l = math.MaxUint32 + 1
k = uint32(l)
fmt.Println("l = ", l)
fmt.Println("k = ", k)
}
$ go run uint.go
k = 4294967295
l = 4294967295
l = 4294967296
k = 0
Yes, casting a uint to uint32 without performing a check to ensure
that the value fall within the acceptable range beforehand would be a
bug in KubeVirt. But if libvirtxml used precisely-sized integers,
such casts would need to be employed much less frequently, and
attempts to carelessly cast uint64 to uint32 would hopefully be
easier to spot and catch during review.
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
