[libvirt] [go PATCH 3/8] Change typedParamsPackNew to use a C allocated array

Stop mixing Go allocated memory with C operations for typed parameters and exclusively rely on C allocated memory. This greatly reduces the number of type casts giving clearer code. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- domain.go | 28 ++++----- typedparams.go | 87 ++++++++++++++------------ typedparams_test.go | 8 ++- typedparams_wrapper.go | 139 +++++++++++++++++++++++++++++++++++++++++ typedparams_wrapper.h | 82 ++++++++++++++++++++++++ 5 files changed, 285 insertions(+), 59 deletions(-) create mode 100644 typedparams_wrapper.go create mode 100644 typedparams_wrapper.h diff --git a/domain.go b/domain.go index b39de47..8f7f030 100644 --- a/domain.go +++ b/domain.go @@ -2104,16 +2104,15 @@ func (d *Domain) BlockCopy(disk string, destxml string, params *DomainBlockCopyP info := getBlockCopyParameterFieldInfo(params) - cparams, gerr := typedParamsPackNew(info) + cparams, cnparams, gerr := typedParamsPackNew(info) if gerr != nil { return gerr } - nparams := len(*cparams) - defer C.virTypedParamsClear((*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.int(nparams)) + defer C.virTypedParamsFree(cparams, cnparams) var err C.virError - ret := C.virDomainBlockCopyWrapper(d.ptr, cdisk, cdestxml, (*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.int(nparams), C.uint(flags), &err) + ret := C.virDomainBlockCopyWrapper(d.ptr, cdisk, cdestxml, cparams, cnparams, C.uint(flags), &err) if ret == -1 { return makeError(&err) } @@ -2375,16 +2374,15 @@ func getMigrateParameterFieldInfo(params *DomainMigrateParameters) map[string]ty func (d *Domain) Migrate3(dconn *Connect, params *DomainMigrateParameters, flags DomainMigrateFlags) (*Domain, error) { info := getMigrateParameterFieldInfo(params) - cparams, gerr := typedParamsPackNew(info) + cparams, cnparams, gerr := typedParamsPackNew(info) if gerr != nil { return nil, gerr } - nparams := len(*cparams) - defer C.virTypedParamsClear((*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.int(nparams)) + defer C.virTypedParamsFree(cparams, cnparams) var err C.virError - ret := C.virDomainMigrate3Wrapper(d.ptr, dconn.ptr, (*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.uint(nparams), C.uint(flags), &err) + ret := C.virDomainMigrate3Wrapper(d.ptr, dconn.ptr, cparams, C.uint(cnparams), C.uint(flags), &err) if ret == nil { return nil, makeError(&err) } @@ -2455,16 +2453,15 @@ func (d *Domain) MigrateToURI3(dconnuri string, params *DomainMigrateParameters, } info := getMigrateParameterFieldInfo(params) - cparams, gerr := typedParamsPackNew(info) + cparams, cnparams, gerr := typedParamsPackNew(info) if gerr != nil { return gerr } - nparams := len(*cparams) - defer C.virTypedParamsClear((*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.int(nparams)) + defer C.virTypedParamsFree(cparams, cnparams) var err C.virError - ret := C.virDomainMigrateToURI3Wrapper(d.ptr, cdconnuri, (*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.uint(nparams), C.uint(flags), &err) + ret := C.virDomainMigrateToURI3Wrapper(d.ptr, cdconnuri, cparams, C.uint(cnparams), C.uint(flags), &err) if ret == -1 { return makeError(&err) } @@ -4272,16 +4269,15 @@ func (d *Domain) SetIOThreadParams(iothreadid uint, params *DomainSetIOThreadPar } info := getSetIOThreadParamsFieldInfo(params) - cparams, gerr := typedParamsPackNew(info) + cparams, cnparams, gerr := typedParamsPackNew(info) if gerr != nil { return gerr } - nparams := len(*cparams) - defer C.virTypedParamsClear((*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.int(nparams)) + defer C.virTypedParamsFree(cparams, cnparams) var err C.virError - ret := C.virDomainSetIOThreadParamsWrapper(d.ptr, C.uint(iothreadid), (*C.virTypedParameter)(unsafe.Pointer(&(*cparams)[0])), C.int(nparams), C.uint(flags), &err) + ret := C.virDomainSetIOThreadParamsWrapper(d.ptr, C.uint(iothreadid), cparams, cnparams, C.uint(flags), &err) if ret == -1 { return makeError(&err) } diff --git a/typedparams.go b/typedparams.go index e8ceae0..a1bdffd 100644 --- a/typedparams.go +++ b/typedparams.go @@ -32,6 +32,7 @@ package libvirt #include <libvirt/virterror.h> #include <stdlib.h> #include <string.h> +#include "typedparams_wrapper.h" */ import "C" @@ -197,66 +198,70 @@ func typedParamsPack(cparams []C.virTypedParameter, infomap map[string]typedPara return typedParamsPackLen(&cparams[0], len(cparams), infomap) } -func typedParamsPackNew(infomap map[string]typedParamsFieldInfo) (*[]C.virTypedParameter, error) { - nparams := 0 - for _, value := range infomap { - if !*value.set { - continue - } +func typedParamsPackNew(infomap map[string]typedParamsFieldInfo) (*C.virTypedParameter, C.int, error) { + var cparams C.virTypedParameterPtr + var nparams C.int + var maxparams C.int - if value.sl != nil { - nparams += len(*value.sl) - } else { - nparams++ - } - } + defer C.virTypedParamsFree(cparams, nparams) - cparams := make([]C.virTypedParameter, nparams) - nparams = 0 - for key, value := range infomap { + for name, value := range infomap { if !*value.set { continue } - cfield := C.CString(key) - defer C.free(unsafe.Pointer(cfield)) - clen := len(key) + 1 - if clen > C.VIR_TYPED_PARAM_FIELD_LENGTH { - clen = C.VIR_TYPED_PARAM_FIELD_LENGTH - } + cname := C.CString(name) + defer C.free(unsafe.Pointer(cname)) if value.sl != nil { + /* We're not actually using virTypedParamsAddStringList, as it is + * easier to avoid creating a 'char **' in Go to hold all the strings. + * We none the less do a version check, because earlier libvirts + * would not expect to see multiple string values in a typed params + * list with the same field name + */ + if C.LIBVIR_VERSION_NUMBER < 1002017 { + return nil, 0, makeNotImplementedError("virTypedParamsAddStringList") + } for i := 0; i < len(*value.sl); i++ { - cparam := &cparams[nparams] - cparam._type = C.VIR_TYPED_PARAM_STRING - C.memcpy(unsafe.Pointer(&cparam.field[0]), unsafe.Pointer(cfield), C.size_t(clen)) - nparams++ + cvalue := C.CString((*value.sl)[i]) + defer C.free(unsafe.Pointer(cvalue)) + var err C.virError + ret := C.virTypedParamsAddStringWrapper(&cparams, &nparams, &maxparams, cname, cvalue, &err) + if ret < 0 { + return nil, 0, makeError(&err) + } } } else { - cparam := &cparams[nparams] + var err C.virError + var ret C.int if value.i != nil { - cparam._type = C.VIR_TYPED_PARAM_INT + ret = C.virTypedParamsAddIntWrapper(&cparams, &nparams, &maxparams, cname, C.int(*value.i), &err) } else if value.ui != nil { - cparam._type = C.VIR_TYPED_PARAM_UINT + ret = C.virTypedParamsAddUIntWrapper(&cparams, &nparams, &maxparams, cname, C.uint(*value.ui), &err) } else if value.l != nil { - cparam._type = C.VIR_TYPED_PARAM_LLONG + ret = C.virTypedParamsAddLLongWrapper(&cparams, &nparams, &maxparams, cname, C.longlong(*value.l), &err) } else if value.ul != nil { - cparam._type = C.VIR_TYPED_PARAM_ULLONG + ret = C.virTypedParamsAddULLongWrapper(&cparams, &nparams, &maxparams, cname, C.ulonglong(*value.ul), &err) } else if value.b != nil { - cparam._type = C.VIR_TYPED_PARAM_BOOLEAN + v := 0 + if *value.b { + v = 1 + } + ret = C.virTypedParamsAddBooleanWrapper(&cparams, &nparams, &maxparams, cname, C.int(v), &err) } else if value.d != nil { - cparam._type = C.VIR_TYPED_PARAM_DOUBLE + ret = C.virTypedParamsAddDoubleWrapper(&cparams, &nparams, &maxparams, cname, C.double(*value.i), &err) } else if value.s != nil { - cparam._type = C.VIR_TYPED_PARAM_STRING + cvalue := C.CString(*value.s) + defer C.free(unsafe.Pointer(cvalue)) + ret = C.virTypedParamsAddStringWrapper(&cparams, &nparams, &maxparams, cname, cvalue, &err) + } else { + return nil, 0, fmt.Errorf("No typed parameter value set for field '%s'", name) + } + if ret < 0 { + return nil, 0, makeError(&err) } - C.memcpy(unsafe.Pointer(&cparam.field[0]), unsafe.Pointer(cfield), C.size_t(clen)) - nparams++ } } - err := typedParamsPack(cparams, infomap) - if err != nil { - C.virTypedParamsClear((*C.virTypedParameter)(unsafe.Pointer(&cparams[0])), C.int(nparams)) - return nil, err - } - return &cparams, nil + return cparams, nparams, nil } diff --git a/typedparams_test.go b/typedparams_test.go index 8bc980a..ff3783b 100644 --- a/typedparams_test.go +++ b/typedparams_test.go @@ -77,12 +77,16 @@ func TestPackUnpack(t *testing.T) { sl: &got3, } - params, err := typedParamsPackNew(infoin) + params, nparams, err := typedParamsPackNew(infoin) if err != nil { + lverr, ok := err.(Error) + if ok && lverr.Code == ERR_NO_SUPPORT { + return + } t.Fatal(err) } - nout, err := typedParamsUnpack(*params, infoout) + nout, err := typedParamsUnpackLen(params, int(nparams), infoout) if err != nil { t.Fatal(err) } diff --git a/typedparams_wrapper.go b/typedparams_wrapper.go new file mode 100644 index 0000000..c0248ce --- /dev/null +++ b/typedparams_wrapper.go @@ -0,0 +1,139 @@ +/* + * This file is part of the libvirt-go project + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * Copyright (C) 2019 Red Hat, Inc. + * + */ + +package libvirt + +/* +#cgo pkg-config: libvirt +#include <assert.h> +#include "typedparams_wrapper.h" + +int +virTypedParamsAddIntWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + int value, + virErrorPtr err) +{ + int ret = virTypedParamsAddInt(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +int +virTypedParamsAddUIntWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + unsigned int value, + virErrorPtr err) +{ + int ret = virTypedParamsAddUInt(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +int +virTypedParamsAddLLongWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + long long value, + virErrorPtr err) +{ + int ret = virTypedParamsAddLLong(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +int +virTypedParamsAddULLongWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + unsigned long long value, + virErrorPtr err) +{ + int ret = virTypedParamsAddULLong(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +int +virTypedParamsAddDoubleWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + double value, + virErrorPtr err) +{ + int ret = virTypedParamsAddDouble(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +int +virTypedParamsAddBooleanWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + int value, + virErrorPtr err) +{ + int ret = virTypedParamsAddBoolean(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +int +virTypedParamsAddStringWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + const char *value, + virErrorPtr err) +{ + int ret = virTypedParamsAddString(params, nparams, maxparams, name, value); + if (ret < 0) { + virCopyLastError(err); + } + return ret; +} + +*/ +import "C" diff --git a/typedparams_wrapper.h b/typedparams_wrapper.h new file mode 100644 index 0000000..d2ef7d6 --- /dev/null +++ b/typedparams_wrapper.h @@ -0,0 +1,82 @@ +/* + * This file is part of the libvirt-go project + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + * + * Copyright (C) 2019 Red Hat, Inc. + * + */ + +#ifndef LIBVIRT_GO_TYPEDPARAMS_WRAPPER_H__ +#define LIBVIRT_GO_TYPEDPARAMS_WRAPPER_H__ + +#include <libvirt/libvirt.h> +#include <libvirt/virterror.h> + +int +virTypedParamsAddIntWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + int value, + virErrorPtr err); +int +virTypedParamsAddUIntWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + unsigned int value, + virErrorPtr err); +int +virTypedParamsAddLLongWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + long long value, + virErrorPtr err); +int +virTypedParamsAddULLongWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + unsigned long long value, + virErrorPtr err); +int +virTypedParamsAddDoubleWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + double value, + virErrorPtr err); +int +virTypedParamsAddBooleanWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + int value, + virErrorPtr err); +int +virTypedParamsAddStringWrapper(virTypedParameterPtr *params, + int *nparams, + int *maxparams, + const char *name, + const char *value, + virErrorPtr err); + +#endif /* LIBVIRT_GO_TYPEDPARAMS_WRAPPER_H__ */ -- 2.20.1