[v2] golang/xenlight: Add libxl_utils support
diff mbox series

Message ID 20190628082508.32509-1-nicolas.belouin@gandi.net
State New, archived
Headers show
Series
  • [v2] golang/xenlight: Add libxl_utils support
Related show

Commit Message

Nicolas Belouin June 28, 2019, 8:25 a.m. UTC
The Go bindings for libxl miss functions from libxl_utils, let's start
with the simple libxl_domid_to_name and its counterpart
libxl_name_to_domid.

Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
---
 tools/golang/xenlight/xenlight_utils.go | 55 +++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 tools/golang/xenlight/xenlight_utils.go

Comments

George Dunlap June 28, 2019, 4:32 p.m. UTC | #1
On 6/28/19 9:25 AM, Nicolas Belouin wrote:
> The Go bindings for libxl miss functions from libxl_utils, let's start
> with the simple libxl_domid_to_name and its counterpart
> libxl_name_to_domid.
> 
> Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>

Just for future reference, below your SoB, it's good practice to put a
`---` line (below which everything will be ignored), and a list of the
changes you made.  E.g,:

Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
---
v2:
- Don't leak C string returned by libxl_domid_to_name

One more thing...

> +//char* libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
> +func (Ctx *Context) DomidToName(id Domid) (name string) {
> +	cDomName := C.libxl_domid_to_name(Ctx.ctx, C.uint32_t(id))
> +	defer C.free(unsafe.Pointer(cDomName))
> +
> +	name = C.GoString(cDomName)

libxl_domid_to_name() returns NULL if domid doesn't exist.  Will this
code DTRT (returning 'nil' in that case)?  Or will it crash / do
something else?

I couldn't actually find the answer in a quick search for the
documentation.  Any chance you could build a test program to see what
happens?

Alternately, we could play it safe and always check cDomName for `nil`
before passing it to C.GoString().

Thanks,
 -George
George Dunlap June 28, 2019, 8:01 p.m. UTC | #2
> On Jun 28, 2019, at 5:32 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> On 6/28/19 9:25 AM, Nicolas Belouin wrote:
>> The Go bindings for libxl miss functions from libxl_utils, let's start
>> with the simple libxl_domid_to_name and its counterpart
>> libxl_name_to_domid.
>> 
>> Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
> 
> Just for future reference, below your SoB, it's good practice to put a
> `---` line (below which everything will be ignored), and a list of the
> changes you made.  E.g,:
> 
> Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
> ---
> v2:
> - Don't leak C string returned by libxl_domid_to_name
> 
> One more thing...
> 
>> +//char* libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
>> +func (Ctx *Context) DomidToName(id Domid) (name string) {
>> +	cDomName := C.libxl_domid_to_name(Ctx.ctx, C.uint32_t(id))
>> +	defer C.free(unsafe.Pointer(cDomName))
>> +
>> +	name = C.GoString(cDomName)
> 
> libxl_domid_to_name() returns NULL if domid doesn't exist.  Will this
> code DTRT (returning 'nil' in that case)?  Or will it crash / do
> something else?
> 
> I couldn't actually find the answer in a quick search for the
> documentation.  Any chance you could build a test program to see what
> happens?
> 
> Alternately, we could play it safe and always check cDomName for `nil`
> before passing it to C.GoString().

I just asked, and it turns out if C.GoString() is passed a nil pointer, it returns the empty string (“”), which is what we want.  It’s not documented yet, but there’s a ticket to document it soon.

https://github.com/golang/go/issues/32734

So this is ready to go in:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>
George Dunlap July 18, 2019, 9:54 p.m. UTC | #3
> On Jun 28, 2019, at 9:01 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> 
> 
>> On Jun 28, 2019, at 5:32 PM, George Dunlap <george.dunlap@citrix.com> wrote:
>> 
>> On 6/28/19 9:25 AM, Nicolas Belouin wrote:
>>> The Go bindings for libxl miss functions from libxl_utils, let's start
>>> with the simple libxl_domid_to_name and its counterpart
>>> libxl_name_to_domid.
>>> 
>>> Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
>> 
>> Just for future reference, below your SoB, it's good practice to put a
>> `---` line (below which everything will be ignored), and a list of the
>> changes you made.  E.g,:
>> 
>> Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
>> ---
>> v2:
>> - Don't leak C string returned by libxl_domid_to_name
>> 
>> One more thing...
>> 
>>> +//char* libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
>>> +func (Ctx *Context) DomidToName(id Domid) (name string) {
>>> +	cDomName := C.libxl_domid_to_name(Ctx.ctx, C.uint32_t(id))
>>> +	defer C.free(unsafe.Pointer(cDomName))
>>> +
>>> +	name = C.GoString(cDomName)
>> 
>> libxl_domid_to_name() returns NULL if domid doesn't exist.  Will this
>> code DTRT (returning 'nil' in that case)?  Or will it crash / do
>> something else?
>> 
>> I couldn't actually find the answer in a quick search for the
>> documentation.  Any chance you could build a test program to see what
>> happens?
>> 
>> Alternately, we could play it safe and always check cDomName for `nil`
>> before passing it to C.GoString().
> 
> I just asked, and it turns out if C.GoString() is passed a nil pointer, it returns the empty string (“”), which is what we want.  It’s not documented yet, but there’s a ticket to document it soon.
> 
> https://github.com/golang/go/issues/32734
> 
> So this is ready to go in:

Actually, turns out it’s not:  You added a file, but it’s not wired into the build system.  You need to add xenlight_utils.go to PKGSOURCES in the Makefile.  I’ve just sent an updated patch.

 -George

Patch
diff mbox series

diff --git a/tools/golang/xenlight/xenlight_utils.go b/tools/golang/xenlight/xenlight_utils.go
new file mode 100644
index 0000000000..da1636842d
--- /dev/null
+++ b/tools/golang/xenlight/xenlight_utils.go
@@ -0,0 +1,55 @@ 
+/*
+ * Copyright (C) 2019 Nicolas Belouin, Gandi SAS
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+package xenlight
+
+/*
+#cgo LDFLAGS: -lxenlight -lyajl -lxentoollog
+#include <stdlib.h>
+#include <libxl_utils.h>
+*/
+import "C"
+
+import (
+	"unsafe"
+)
+
+//char* libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
+func (Ctx *Context) DomidToName(id Domid) (name string) {
+	cDomName := C.libxl_domid_to_name(Ctx.ctx, C.uint32_t(id))
+	defer C.free(unsafe.Pointer(cDomName))
+
+	name = C.GoString(cDomName)
+	return
+}
+
+//int libxl_name_to_domid(libxl_ct *ctx, const char *name, uint32_t *domid)
+func (Ctx *Context) NameToDomid(name string) (id Domid, err error) {
+	cname := C.CString(name)
+	defer C.free(unsafe.Pointer(cname))
+
+	var cDomId C.uint32_t
+
+	ret := C.libxl_name_to_domid(Ctx.ctx, cname, &cDomId)
+	if ret != 0 {
+		err = Error(-ret)
+		return
+	}
+
+	id = Domid(cDomId)
+
+	return
+}