Mailing List Archive: [PATCH v2 1/3] tools/golang: When returning pointers, actually allocate structrues

[PATCH v2 1/3] tools/golang: When returning pointers, actually allocate structrues

Apr 19, 2024, 6:51 AM

Post #1 of 3 (18 views)

In a handful of cases, it was decided to return a pointer to a
structure rather than the plain structure itself, due to the size.
However, in these cases the structure was never allocated, leading to
a nil pointer exception when calling the relevant `fromC` method.

Allocate structures before attempting to fill them in.

Fixes: 453713b1750 ("golang/xenlight: Add host-related functionality")
Reported-by: Tobias Fitschen <tobias.fitschen@posteo.de>
Signed-off-by: George Dunlap <george.dunlap@cloud.com>
Tested-by: Tobias Fitschen <tobias.fitschen@posteo.de>
---
v2:
- Added Fixes: tag
- Added Tested-by tag

NB this is a candidate for backport.

CC: Nick Rosbrook <rosbrookn@gmail.com>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
tools/golang/xenlight/xenlight.go | 3 +++
1 file changed, 3 insertions(+)

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index a45c636952..d793f172e5 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -999,6 +999,7 @@ func (ctx *Context) GetPhysinfo() (physinfo *Physinfo, err error) {
err = Error(ret)
return
}
+ physinfo = &Physinfo{}
err = physinfo.fromC(&cphys)

return
@@ -1010,6 +1011,7 @@ func (ctx *Context) GetVersionInfo() (info *VersionInfo, err error) {

cinfo = C.libxl_get_version_info(ctx.ctx)

+ info = &VersionInfo{}
err = info.fromC(cinfo)

return
@@ -1027,6 +1029,7 @@ func (ctx *Context) DomainInfo(Id Domid) (di *Dominfo, err error) {
return
}

+ di = &Dominfo{}
err = di.fromC(&cdi)

return
--
2.25.1

Re: [PATCH v2 1/3] tools/golang: When returning pointers, actually allocate structrues [ In reply to ]

rosbrookn at gmail

Apr 19, 2024, 8:27 AM

Post #2 of 3 (18 views)

Permalink

On Fri, Apr 19, 2024 at 10:00?AM George Dunlap <george.dunlap@cloud.com> wrote:
>
> In a handful of cases, it was decided to return a pointer to a
> structure rather than the plain structure itself, due to the size.
> However, in these cases the structure was never allocated, leading to
> a nil pointer exception when calling the relevant `fromC` method.
>
> Allocate structures before attempting to fill them in.
>
> Fixes: 453713b1750 ("golang/xenlight: Add host-related functionality")
> Reported-by: Tobias Fitschen <tobias.fitschen@posteo.de>
> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> Tested-by: Tobias Fitschen <tobias.fitschen@posteo.de>

Acked-by: Nick Rosbrook <rosbrookn@gmail.com>

This is one of the reasons I don't like using named return values (if
you can help it) and naked returns. When you declare the variable
explicitly you are less likely to explicitly initialize it to nil. Or,
it's a compile time error if you forget all together.

Thanks,
Nick

Re: [PATCH v2 1/3] tools/golang: When returning pointers, actually allocate structrues [ In reply to ]

george.dunlap at cloud

Apr 19, 2024, 8:57 AM

Post #3 of 3 (18 views)

Permalink

On Fri, Apr 19, 2024 at 4:27?PM Nick Rosbrook <rosbrookn@gmail.com> wrote:
>
> On Fri, Apr 19, 2024 at 10:00?AM George Dunlap <george.dunlap@cloud.com> wrote:
> >
> > In a handful of cases, it was decided to return a pointer to a
> > structure rather than the plain structure itself, due to the size.
> > However, in these cases the structure was never allocated, leading to
> > a nil pointer exception when calling the relevant `fromC` method.
> >
> > Allocate structures before attempting to fill them in.
> >
> > Fixes: 453713b1750 ("golang/xenlight: Add host-related functionality")
> > Reported-by: Tobias Fitschen <tobias.fitschen@posteo.de>
> > Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> > Tested-by: Tobias Fitschen <tobias.fitschen@posteo.de>
>
> Acked-by: Nick Rosbrook <rosbrookn@gmail.com>
>
> This is one of the reasons I don't like using named return values (if
> you can help it) and naked returns. When you declare the variable
> explicitly you are less likely to explicitly initialize it to nil. Or,
> it's a compile time error if you forget all together.

Yes, after having more experience in golang I wouldn't be opposed to
getting rid of all those now. But as I'm hoping to backport this as a
bug fix, that would be a subsequent patch. :-)

Thanks,
-George