On 31. 3. 2020 17:44, Seeteena Thoufeek wrote:
> Signed-off-by: Seeteena Thoufeek <s1seetee@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_cgroup.c | 91 +++++++++++++++++++-------------------------------
> 1 file changed, 35 insertions(+), 56 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index c0e30f6..d34c515 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -62,7 +62,7 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> int perms = VIR_CGROUP_DEVICE_READ;
> - char **targetPaths = NULL;
> + VIR_AUTOSTRINGLIST targetPaths = NULL;
> size_t i;
> int rv;
> int ret = -1;> @@ -82,12 +82,11 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
> virCgroupGetDevicePermsString(perms),
> rv);
> if (rv < 0)
> - goto cleanup;
> + return ret;
I don't quite understand. Why not return -1 immediatelly? This also
applies to the test of the patch.
>
> if (rv > 0) {
> /* @path is neither character device nor block device. */
> - ret = 0;
> - goto cleanup;
> + return 0;
> }
>
> if (virDevMapperGetTargets(path, &targetPaths) < 0 &&
> @@ -964,16 +956,9 @@ qemuInitCgroup(virDomainObjPtr vm,
> cfg->maxThreadsPerProc,
> &priv->cgroup) < 0) {
> if (virCgroupNewIgnoreError())
> - goto done;
> -
> - goto cleanup;
> + return 0;
So previouslly, it this has failed, then if the error was to be ingored
then 0 was returned and -1 if it wasn't. But with this change 0 is
returned no matter what.
> }
> -
> - done:
> - ret = 0;
> - cleanup:
> - virObjectUnref(cfg);
> - return ret;
> + return ret;
I don't think this is right. Previously, in success path ret was set to
0. But you removed that, so this returns -1, allways.
> }
>
> static void
> @@ -1058,14 +1043,14 @@ int
> qemuConnectCgroup(virDomainObjPtr vm)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(priv->driver);
> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
> int ret = -1;
>
> if (!virQEMUDriverIsPrivileged(priv->driver))
> - goto done;
> + return 0;
>
> if (!virCgroupAvailable())
> - goto done;
> + return 0;
>
> virCgroupFree(&priv->cgroup);
>
> @@ -1075,14 +1060,9 @@ qemuConnectCgroup(virDomainObjPtr vm)
> cfg->cgroupControllers,
> priv->machineName,
> &priv->cgroup) < 0)
> - goto cleanup;
> + return ret;
>
> qemuRestoreCgroupState(vm);
> -
> - done:
> - ret = 0;
> - cleanup:
> - virObjectUnref(cfg);
> return ret;
> }
>
> @@ -1269,7 +1249,7 @@ qemuCgroupEmulatorAllNodesAllow(virCgroupPtr cgroup,
> qemuCgroupEmulatorAllNodesDataPtr *retData)
> {
> qemuCgroupEmulatorAllNodesDataPtr data = NULL;
> - char *all_nodes_str = NULL;
> + g_autofree char *all_nodes_str = NULL;
> virBitmapPtr all_nodes = NULL;
virBitmap can be made g_autoptr() too.
> int ret = -1;
>
> @@ -1298,7 +1278,6 @@ qemuCgroupEmulatorAllNodesAllow(virCgroupPtr cgroup,
> ret = 0;
>
> cleanup:
> - VIR_FREE(all_nodes_str);
> virBitmapFree(all_nodes);
> qemuCgroupEmulatorAllNodesDataFree(data);
>
>
Nevertheless, I'm fixing all the issues, ACKing and pushing.
Michal
