[libvirt] [PATCH 0/2] libxl: some small fixes
I noticed a few small bugs while fixing the domain maximum memory bugs :-). See patches for details. Jim Fehlig (2): libxl: honor autoballoon setting in libxl.conf libxl: use init and dispose functions with libxl_physinfo src/libxl/libxl_capabilities.c | 16 +++++++++++----- src/libxl/libxl_conf.c | 7 ++++++- src/libxl/libxl_driver.c | 2 ++ 3 files changed, 19 insertions(+), 6 deletions(-) -- 2.9.2
libxlGetAutoballoonConf is supposed to honor user-specified autoballoon setting in libxl.conf. As written, the user-specified setting could be overwritten by the subsequent logic to check dom0_mem parameter. If user-specified setting is present and correct, accept it. Only fallback to checking Xen dom0_mem command line parameter if user-specfied setting is not present. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b5186f2..ee1e639 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1355,8 +1355,11 @@ libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, regex_t regex; int res; - if (virConfGetValueBool(conf, "autoballoon", &cfg->autoballoon) < 0) + res = virConfGetValueBool(conf, "autoballoon", &cfg->autoballoon); + if (res < 0) return -1; + else if (res == 1) + return 0; if ((res = regcomp(®ex, "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )", -- 2.9.2
On 02/02/2017 10:39 PM, Jim Fehlig wrote:
libxlGetAutoballoonConf is supposed to honor user-specified autoballoon setting in libxl.conf. As written, the user-specified setting could be overwritten by the subsequent logic to check dom0_mem parameter. If user-specified setting is present and correct, accept it. Only fallback to checking Xen dom0_mem command line parameter if user-specfied setting is not present.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Nice catch!
Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
--- src/libxl/libxl_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b5186f2..ee1e639 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1355,8 +1355,11 @@ libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, regex_t regex; int res;
- if (virConfGetValueBool(conf, "autoballoon", &cfg->autoballoon) < 0) + res = virConfGetValueBool(conf, "autoballoon", &cfg->autoballoon); + if (res < 0) return -1; + else if (res == 1) + return 0;
if ((res = regcomp(®ex, "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )",
The typical pattern when calling libxl functions that populate a structure is libxl_foo foo; libxl_foo_init(&foo); libxl_get_foo(ctx, &foo); ... libxl_foo_dispose(&foo); Fix several instances of libxl_physinfo missing the init and dispose calls. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_capabilities.c | 16 +++++++++++----- src/libxl/libxl_conf.c | 2 ++ src/libxl/libxl_driver.c | 2 ++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index a1c0a87..2bbd2d1 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -211,27 +211,33 @@ libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps) const libxl_version_info *ver_info; enum libxlHwcapVersion version; libxl_physinfo phy_info; + int ret = -1; + libxl_physinfo_init(&phy_info); if (libxl_get_physinfo(ctx, &phy_info) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to get node physical info from libxenlight")); - return -1; + goto cleanup; } if ((ver_info = libxl_get_version_info(ctx)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to get version info from libxenlight")); - return -1; + goto cleanup; } version = (ver_info->xen_version_minor >= 7); if (libxlCapsInitCPU(caps, &phy_info, version) < 0) - return -1; + goto cleanup; if (virCapabilitiesSetNetPrefix(caps, LIBXL_GENERATED_PREFIX_XEN) < 0) - return -1; + goto cleanup; - return 0; + ret = 0; + + cleanup: + libxl_physinfo_dispose(&phy_info); + return ret; } static int diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index ee1e639..28b05e1 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1943,6 +1943,7 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); int ret = -1; + libxl_physinfo_init(&phy_info); if (libxl_get_physinfo(cfg->ctx, &phy_info)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxl_get_physinfo_info failed")); @@ -1967,6 +1968,7 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) ret = 0; cleanup: + libxl_physinfo_dispose(&phy_info); virObjectUnref(cfg); return ret; } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3a69720..8951bef 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) if (virNodeGetFreeMemoryEnsureACL(conn) < 0) goto cleanup; + libxl_physinfo_init(&phy_info); if (libxl_get_physinfo(cfg->ctx, &phy_info)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxl_get_physinfo_info failed")); @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) ret = phy_info.free_pages * cfg->verInfo->pagesize; cleanup: + libxl_physinfo_dispose(&phy_info); virObjectUnref(cfg); return ret; } -- 2.9.2
On 02/02/2017 10:39 PM, Jim Fehlig wrote:
The typical pattern when calling libxl functions that populate a structure is
libxl_foo foo; libxl_foo_init(&foo); libxl_get_foo(ctx, &foo); ... libxl_foo_dispose(&foo);
Fix several instances of libxl_physinfo missing the init and dispose calls. Indeed,
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Joao Martins <joao.m.martins@oracle.com> See also one comment/nit below, perhaps one libxl_physinfo_init could be moved slightly up..
[...]
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3a69720..8951bef 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) if (virNodeGetFreeMemoryEnsureACL(conn) < 0) goto cleanup;
+ libxl_physinfo_init(&phy_info);
.. namely here? That is before virNodeGetFreeMemoryEnsureACL. Not that it matters much, as init/dispose in this case just zeroes out phy_info region. But just perhaps consistency as you would end disposing an non initialized object.
if (libxl_get_physinfo(cfg->ctx, &phy_info)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxl_get_physinfo_info failed")); @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) ret = phy_info.free_pages * cfg->verInfo->pagesize;
cleanup: + libxl_physinfo_dispose(&phy_info); virObjectUnref(cfg); return ret; }
Joao Martins wrote:
On 02/02/2017 10:39 PM, Jim Fehlig wrote:
The typical pattern when calling libxl functions that populate a structure is
libxl_foo foo; libxl_foo_init(&foo); libxl_get_foo(ctx, &foo); ... libxl_foo_dispose(&foo);
Fix several instances of libxl_physinfo missing the init and dispose calls. Indeed,
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
See also one comment/nit below, perhaps one libxl_physinfo_init could be moved slightly up..
[...]
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3a69720..8951bef 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) if (virNodeGetFreeMemoryEnsureACL(conn) < 0) goto cleanup;
+ libxl_physinfo_init(&phy_info);
.. namely here? That is before virNodeGetFreeMemoryEnsureACL.
Nice catch. Moved as suggested in my local branch. Any other comments on this small series? Would be nice to get these bug fixes committed :-). Regards, Jim
Not that it matters much, as init/dispose in this case just zeroes out phy_info region. But just perhaps consistency as you would end disposing an non initialized object.
if (libxl_get_physinfo(cfg->ctx, &phy_info)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxl_get_physinfo_info failed")); @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) ret = phy_info.free_pages * cfg->verInfo->pagesize;
cleanup: + libxl_physinfo_dispose(&phy_info); virObjectUnref(cfg); return ret; }
On 02/08/2017 04:17 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 02/02/2017 10:39 PM, Jim Fehlig wrote:
The typical pattern when calling libxl functions that populate a structure is
libxl_foo foo; libxl_foo_init(&foo); libxl_get_foo(ctx, &foo); ... libxl_foo_dispose(&foo);
Fix several instances of libxl_physinfo missing the init and dispose calls. Indeed,
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
See also one comment/nit below, perhaps one libxl_physinfo_init could be moved slightly up..
[...]
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3a69720..8951bef 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) if (virNodeGetFreeMemoryEnsureACL(conn) < 0) goto cleanup;
+ libxl_physinfo_init(&phy_info);
.. namely here? That is before virNodeGetFreeMemoryEnsureACL.
Nice catch. Moved as suggested in my local branch.
Any other comments on this small series? Would be nice to get these bug fixes committed :-).
Nope, looks all good to me: Acked-by: Joao Martins <joao.m.martins@oracle.com>
Regards, Jim
Not that it matters much, as init/dispose in this case just zeroes out phy_info region. But just perhaps consistency as you would end disposing an non initialized object.
if (libxl_get_physinfo(cfg->ctx, &phy_info)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxl_get_physinfo_info failed")); @@ -4295,6 +4296,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) ret = phy_info.free_pages * cfg->verInfo->pagesize;
cleanup: + libxl_physinfo_dispose(&phy_info); virObjectUnref(cfg); return ret; }
Joao Martins wrote:
On 02/08/2017 04:17 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 02/02/2017 10:39 PM, Jim Fehlig wrote:
The typical pattern when calling libxl functions that populate a structure is
libxl_foo foo; libxl_foo_init(&foo); libxl_get_foo(ctx, &foo); ... libxl_foo_dispose(&foo);
Fix several instances of libxl_physinfo missing the init and dispose calls. Indeed,
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
See also one comment/nit below, perhaps one libxl_physinfo_init could be moved slightly up..
[...] diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3a69720..8951bef 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4286,6 +4286,7 @@ libxlNodeGetFreeMemory(virConnectPtr conn) if (virNodeGetFreeMemoryEnsureACL(conn) < 0) goto cleanup;
+ libxl_physinfo_init(&phy_info); .. namely here? That is before virNodeGetFreeMemoryEnsureACL. Nice catch. Moved as suggested in my local branch.
Any other comments on this small series? Would be nice to get these bug fixes committed :-).
Nope, looks all good to me:
Acked-by: Joao Martins <joao.m.martins@oracle.com>
Thanks, I've pushed these fixes now. Regards, Jim
participants (2)
-
Jim Fehlig -
Joao Martins