[libvirt] [PATCH 00/42] Cleanups after adopting g_get_*_dir()
After adopting g_get_*_dir(), internally, when implementing virGetUser*Dir(), the libvirt functions changed their behaviour as NULL is never ever returned by the GLib functions. Knowing that, let's cleanup the callers' code of those functions, removing the unnecessary checks. While doing the cleanup mentioned above, I've noticed that some other cleanups could be done, when touching the very same function, as using g_autofree. It's done as part of this series as well. phyp driver was not touched as there's some plan to just drop its code entirely: https://www.redhat.com/archives/libvir-list/2019-December/msg01162.html Fabiano Fidêncio (42): tools: Use g_autofree on cmdCd() vbox: Don't leak virGetUserDirectory()'s output rpc: Use g_autofree on virNetClientNewLibSSH2() rpc: Use g_autofree on virNetClientNewLibssh() rpc: Don't check the output of virGetUserDirectory() qemu: Don't check the output of virGetUserDirectory() util: Don't check the output of virGetUserConfigDirectory() storage: Don't check the output of virGetUserConfigDirectory() secret: Don't check the output of virGetUserConfigDirectory() tools: Don't check the output of virGetUserCacheDirectory() vbox: Use g_autofree on vboxDomainScreenshot() vbox: Don't check the output of virGetUserCacheDirectory() util: Use g_autofree on virLogSetDefaultOutputToFile() util: Don't check the output of virGetUserCacheDirectory() qemu: Don't check the output of virGetUserCacheDirectory() rpc: Don't check the output of virGetUserConfigDirectory() remote: Don't check the output of virGetUserConfigDirectory() qemu: Don't check the output of virGetUserConfigDirectory() network: Don't check the output of virGetUserConfigDirectory() logging: Use g_autofree on virLogDaemonConfigFilePath() logging: Don't check the output of virGetUserConfigDirectory() locking: Use g_autofree on virLockDaemonConfigFilePath() locking: Don't check the output of virGetUserConfigDirectory() util: Don't check the output of virGetUserRuntimeDirectory() storage: Don't check the output of virGetUserRuntimeDirectory() secret: Don't check the output of virGetUserRuntimeDirectory() rpc: Don't check the output of virGetUserRuntimeDirectory() remote: Don't check the output of virGetUserRuntimeDirectory() qemu: Don't check the output of virGetUserRuntimeDirectory() node_device: Don't check the output of virGetUserRuntimeDirectory() network: Don't check the output of virGetUserRuntimeDirectory() logging: Use g_autofree on virLogManagerDaemonPath() logging: Use g_autofree on virLogDaemonUnixSocketPaths() logging: Use g_autofree on virLogDaemonExecRestartStatePath() logging: Don't check the output of virGetUserRuntimeDirectory() locking: Use g_autofree on virLockManagerLockDaemonPath() locking: Use g_autofree on virLockDaemonUnixSocketPaths() locking: Use g_autofree on virLockDaemonExecRestartStatePath() locking: Don't check the output of virGetUserRuntimeDirectory() interface: Don't check the output of virGetUserRuntimeDirectory() admin: Use g_autofree on getSocketPath() admin: Don't check the output of virGetUserRuntimeDirectory() src/admin/libvirt-admin.c | 6 +-- src/interface/interface_backend_netcf.c | 3 +- src/interface/interface_backend_udev.c | 3 +- src/locking/lock_daemon.c | 31 +++---------- src/locking/lock_daemon_config.c | 9 +--- src/locking/lock_driver_lockd.c | 7 +-- src/logging/log_daemon.c | 31 +++---------- src/logging/log_daemon_config.c | 9 +--- src/logging/log_manager.c | 7 +-- src/network/bridge_driver.c | 2 - src/node_device/node_device_hal.c | 3 +- src/node_device/node_device_udev.c | 3 +- src/qemu/qemu_conf.c | 7 +-- src/qemu/qemu_interop_config.c | 3 -- src/remote/remote_daemon.c | 8 +--- src/remote/remote_daemon_config.c | 6 +-- src/remote/remote_driver.c | 3 +- src/rpc/virnetclient.c | 59 +++++++++---------------- src/rpc/virnetsocket.c | 3 +- src/rpc/virnettlscontext.c | 12 ----- src/secret/secret_driver.c | 6 +-- src/storage/storage_driver.c | 2 - src/util/virauth.c | 3 +- src/util/virconf.c | 2 - src/util/virhostdev.c | 3 +- src/util/virlog.c | 13 ++---- src/util/virpidfile.c | 3 +- src/vbox/vbox_common.c | 12 ++--- src/vbox/vbox_storage.c | 7 ++- tools/vsh.c | 13 ++---- 30 files changed, 73 insertions(+), 206 deletions(-) -- 2.24.1
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- tools/vsh.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 5ccda5ab44..bbb6227130 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3175,8 +3175,7 @@ bool cmdCd(vshControl *ctl, const vshCmd *cmd) { const char *dir = NULL; - char *dir_malloced = NULL; - bool ret = true; + g_autofree char *dir_malloced = NULL; char ebuf[1024]; if (!ctl->imode) { @@ -3192,11 +3191,10 @@ cmdCd(vshControl *ctl, const vshCmd *cmd) if (chdir(dir) == -1) { vshError(ctl, _("cd: %s: %s"), virStrerror(errno, ebuf, sizeof(ebuf)), dir); - ret = false; + return false; } - VIR_FREE(dir_malloced); - return ret; + return true; } const vshCmdOptDef opts_echo[] = { -- 2.24.1
In the commit summary: s/on/in/ On Thu, Dec 19, 2019 at 11:04:06AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- tools/vsh.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On vboxStorageVolCreateXML(), virGetUserDirectory() was called without freeing its content later on. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_storage.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c index 790b0e4997..7f240c5333 100644 --- a/src/vbox/vbox_storage.c +++ b/src/vbox/vbox_storage.c @@ -410,6 +410,7 @@ vboxStorageVolCreateXML(virStoragePoolPtr pool, resultCodeUnion resultCode; virStorageVolPtr ret = NULL; g_autoptr(virStorageVolDef) def = NULL; + g_autofree char *homedir = NULL; if (!data->vboxObj) return ret; @@ -442,8 +443,10 @@ vboxStorageVolCreateXML(virStoragePoolPtr pool, } /* If target.path isn't given, use default path ~/.VirtualBox/image_name */ - if (!def->target.path) - def->target.path = g_strdup_printf("%s/.VirtualBox/%s", virGetUserDirectory(), def->name); + if (!def->target.path) { + homedir = virGetUserDirectory(); + def->target.path = g_strdup_printf("%s/.VirtualBox/%s", homedir, def->name); + } VBOX_UTF8_TO_UTF16(def->target.path, &hddNameUtf16); if (!hddFormatUtf16 || !hddNameUtf16) -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:07AM +0100, Fabiano Fidêncio wrote:
On vboxStorageVolCreateXML(), virGetUserDirectory() was called without
s/On/In/
freeing its content later on.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_storage.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 47a17d30f7..75e653fec8 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -442,13 +442,13 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, virNetClientPtr ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - char *nc = NULL; - char *command = NULL; + g_autofree char *nc = NULL; + g_autofree char *command = NULL; - char *homedir = NULL; - char *confdir = NULL; - char *knownhosts = NULL; - char *privkey = NULL; + g_autofree char *homedir = NULL; + g_autofree char *confdir = NULL; + g_autofree char *knownhosts = NULL; + g_autofree char *privkey = NULL; /* Use default paths for known hosts an public keys if not provided */ if (knownHostsPath) { @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, goto cleanup; cleanup: - VIR_FREE(command); - VIR_FREE(privkey); - VIR_FREE(knownhosts); - VIR_FREE(homedir); - VIR_FREE(confdir); - VIR_FREE(nc); return ret; no_memory: -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:08AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
[...]
if (knownHostsPath) { @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, goto cleanup;
cleanup: - VIR_FREE(command); - VIR_FREE(privkey); - VIR_FREE(knownhosts); - VIR_FREE(homedir); - VIR_FREE(confdir); - VIR_FREE(nc); return ret;
This will leave a now unneeded 'cleanup' label: cleanup: return ret; In a quick glance at the patch series I noticed that Patch 41 also leaves an unused 'cleanup' label as well after the changes. Thanks, DHB
no_memory:
On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
[...]
if (knownHostsPath) { @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, goto cleanup;
cleanup: - VIR_FREE(command); - VIR_FREE(privkey); - VIR_FREE(knownhosts); - VIR_FREE(homedir); - VIR_FREE(confdir); - VIR_FREE(nc); return ret;
This will leave a now unneeded 'cleanup' label:
cleanup: return ret;
In a quick glance at the patch series I noticed that Patch 41 also leaves an unused 'cleanup' label as well after the changes.
Daniel, Please, take a look at the no_memory label. It calls the cleanup one. Best Regards, -- Fabiano Fidêncio
On 12/19/19 7:17 PM, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
[...]
if (knownHostsPath) { @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, goto cleanup;
cleanup: - VIR_FREE(command); - VIR_FREE(privkey); - VIR_FREE(knownhosts); - VIR_FREE(homedir); - VIR_FREE(confdir); - VIR_FREE(nc); return ret;
This will leave a now unneeded 'cleanup' label:
cleanup: return ret;
In a quick glance at the patch series I noticed that Patch 41 also leaves an unused 'cleanup' label as well after the changes.
Daniel,
Please, take a look at the no_memory label. It calls the cleanup one.
I am afraid I wasn't clear. By 'unneeded' I mean a label that is a simply 'return' call. 'cleanup' is doing no cleanups anymore. So this this guy: no_memory: virReportOOMError(); goto cleanup; turns into: no_memory: virReportOOMError(); return ret; And in fact, since ret is initialized with NULL, and it will never be set before a no_memory jump, you can even do: no_memory: virReportOOMError(); return NULL; All 'cleanup' jumps can be changed to 'return NULL' and the line that sets 'ret' can turn into return virNetClientNew(sock, NULL)); And you can get rid of both the 'cleanup' label and the 'ret' variable. All of this is just me being pedantic though :) I saw that you erased some 'exit' labels in other patches and wanted to point out that this label is also removable. In the end I believe you can keep this patch just adding g_autofree and removing VIR_FREE calls and, if you want, remove these labels in a separated patch. Thanks, DHB
Best Regards,
On Fri, Dec 20, 2019 at 12:30 AM Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 12/19/19 7:17 PM, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 8:00 PM Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 12/19/19 7:04 AM, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
[...]
if (knownHostsPath) { @@ -517,12 +517,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, goto cleanup;
cleanup: - VIR_FREE(command); - VIR_FREE(privkey); - VIR_FREE(knownhosts); - VIR_FREE(homedir); - VIR_FREE(confdir); - VIR_FREE(nc); return ret;
This will leave a now unneeded 'cleanup' label:
cleanup: return ret;
In a quick glance at the patch series I noticed that Patch 41 also leaves an unused 'cleanup' label as well after the changes.
Daniel,
Please, take a look at the no_memory label. It calls the cleanup one.
I am afraid I wasn't clear. By 'unneeded' I mean a label that is a simply 'return' call. 'cleanup' is doing no cleanups anymore. So this this guy:
no_memory: virReportOOMError(); goto cleanup;
turns into:
no_memory: virReportOOMError(); return ret;
And in fact, since ret is initialized with NULL, and it will never be set before a no_memory jump, you can even do:
no_memory: virReportOOMError(); return NULL;
All 'cleanup' jumps can be changed to 'return NULL' and the line that sets 'ret' can turn into
return virNetClientNew(sock, NULL));
And you can get rid of both the 'cleanup' label and the 'ret' variable.
All of this is just me being pedantic though :) I saw that you erased some 'exit' labels in other patches and wanted to point out that this label is also removable. In the end I believe you can keep this patch just adding g_autofree and removing VIR_FREE calls and, if you want, remove these labels in a separated patch.
I see. I'll approach this in a different series, tho. Best Regards, -- Fabiano Fidêncio
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 75e653fec8..1b882a261a 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -545,13 +545,13 @@ virNetClientPtr virNetClientNewLibssh(const char *host, virNetClientPtr ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - char *nc = NULL; - char *command = NULL; + g_autofree char *nc = NULL; + g_autofree char *command = NULL; - char *homedir = NULL; - char *confdir = NULL; - char *knownhosts = NULL; - char *privkey = NULL; + g_autofree char *homedir = NULL; + g_autofree char *confdir = NULL; + g_autofree char *knownhosts = NULL; + g_autofree char *privkey = NULL; /* Use default paths for known hosts an public keys if not provided */ if (knownHostsPath) { @@ -609,12 +609,6 @@ virNetClientPtr virNetClientNewLibssh(const char *host, goto cleanup; cleanup: - VIR_FREE(command); - VIR_FREE(privkey); - VIR_FREE(knownhosts); - VIR_FREE(homedir); - VIR_FREE(confdir); - VIR_FREE(nc); return ret; no_memory: -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:09AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 1b882a261a..40e5fa35e2 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -466,10 +466,8 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, privkey = g_strdup(privkeyPath); } else { homedir = virGetUserDirectory(); - if (homedir) { - if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0) - goto no_memory; - } + if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0) + goto no_memory; } if (!authMethods) { @@ -566,10 +564,8 @@ virNetClientPtr virNetClientNewLibssh(const char *host, privkey = g_strdup(privkeyPath); } else { homedir = virGetUserDirectory(); - if (homedir) { - if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0) - goto no_memory; - } + if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0) + goto no_memory; } if (!authMethods) { diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index ec9dd35c46..08944f6771 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, */ userdir = virGetUserDirectory(); - if (!userdir) - goto error; - user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir); VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path); @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, VIR_FREE(userdir); return 0; - - error: - VIR_FREE(*cacert); - VIR_FREE(*cacrl); - VIR_FREE(*key); - VIR_FREE(*cert); - VIR_FREE(user_pki_path); - VIR_FREE(userdir); - return -1; } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-)
[...]
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index ec9dd35c46..08944f6771 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, */ userdir = virGetUserDirectory();
- if (!userdir) - goto error; - user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path); @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, VIR_FREE(userdir);
return 0; - - error: - VIR_FREE(*cacert); - VIR_FREE(*cacrl); - VIR_FREE(*key); - VIR_FREE(*cert); - VIR_FREE(user_pki_path); - VIR_FREE(userdir); - return -1;
This doesn't look right. Looks like some leftover from rebase where you wanted to use g_autofree. Pavel
On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-)
[...]
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index ec9dd35c46..08944f6771 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, */ userdir = virGetUserDirectory();
- if (!userdir) - goto error; - user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path); @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, VIR_FREE(userdir);
return 0; - - error: - VIR_FREE(*cacert); - VIR_FREE(*cacrl); - VIR_FREE(*key); - VIR_FREE(*cert); - VIR_FREE(user_pki_path); - VIR_FREE(userdir); - return -1;
This doesn't look right. Looks like some leftover from rebase where you wanted to use g_autofree.
No, actually not. The only usage of `goto error` was when checking the result of virGetUserDirectory(). By removing the check, we have also to remove the label. Best Regards, -- Fabiano Fidêncio
On Thu, Dec 19, 2019 at 05:21:21PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-)
[...]
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index ec9dd35c46..08944f6771 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, */ userdir = virGetUserDirectory();
- if (!userdir) - goto error; - user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path); @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, VIR_FREE(userdir);
return 0; - - error: - VIR_FREE(*cacert); - VIR_FREE(*cacrl); - VIR_FREE(*key); - VIR_FREE(*cert); - VIR_FREE(user_pki_path); - VIR_FREE(userdir); - return -1;
This doesn't look right. Looks like some leftover from rebase where you wanted to use g_autofree.
No, actually not. The only usage of `goto error` was when checking the result of virGetUserDirectory(). By removing the check, we have also to remove the label.
Label yes, the VIR_FREE no as it would leak the memory. Pavel
On Thu, Dec 19, 2019 at 05:25:59PM +0100, Pavel Hrdina wrote:
On Thu, Dec 19, 2019 at 05:21:21PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-)
[...]
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index ec9dd35c46..08944f6771 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, */ userdir = virGetUserDirectory();
- if (!userdir) - goto error; - user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path); @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, VIR_FREE(userdir);
return 0; - - error: - VIR_FREE(*cacert); - VIR_FREE(*cacrl); - VIR_FREE(*key); - VIR_FREE(*cert); - VIR_FREE(user_pki_path); - VIR_FREE(userdir); - return -1;
This doesn't look right. Looks like some leftover from rebase where you wanted to use g_autofree.
No, actually not. The only usage of `goto error` was when checking the result of virGetUserDirectory(). By removing the check, we have also to remove the label.
Label yes, the VIR_FREE no as it would leak the memory.
There's "return 0;" right before that label. Jano
Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Dec 19, 2019 at 05:42:14PM +0100, Ján Tomko wrote:
On Thu, Dec 19, 2019 at 05:25:59PM +0100, Pavel Hrdina wrote:
On Thu, Dec 19, 2019 at 05:21:21PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:07 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-)
[...]
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index ec9dd35c46..08944f6771 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -805,9 +805,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, */ userdir = virGetUserDirectory();
- if (!userdir) - goto error; - user_pki_path = g_strdup_printf("%s/.pki/libvirt", userdir);
VIR_DEBUG("Trying to find TLS user credentials in %s", user_pki_path); @@ -864,15 +861,6 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, VIR_FREE(userdir);
return 0; - - error: - VIR_FREE(*cacert); - VIR_FREE(*cacrl); - VIR_FREE(*key); - VIR_FREE(*cert); - VIR_FREE(user_pki_path); - VIR_FREE(userdir); - return -1;
This doesn't look right. Looks like some leftover from rebase where you wanted to use g_autofree.
No, actually not. The only usage of `goto error` was when checking the result of virGetUserDirectory(). By removing the check, we have also to remove the label.
Label yes, the VIR_FREE no as it would leak the memory.
There's "return 0;" right before that label.
Oh, I see, two strings are freed and the remaining ones are transferred to caller. Somehow I missed that. Pavel
On Thu, Dec 19, 2019 at 11:04:10AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 12 ++++-------- src/rpc/virnettlscontext.c | 12 ------------ 2 files changed, 4 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_interop_config.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c index cbabde45df..f5f419e630 100644 --- a/src/qemu/qemu_interop_config.c +++ b/src/qemu/qemu_interop_config.c @@ -118,9 +118,6 @@ qemuInteropFetchConfigs(const char *name, if (!xdgConfig) { g_autofree char *home = virGetUserDirectory(); - if (!home) - return -1; - xdgConfig = g_strdup_printf("%s/.config", home); } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:11AM +0100, Fabiano Fidêncio wrote:
virGetUserDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_interop_config.c | 3 --- 1 file changed, 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virauth.c | 3 +-- src/util/virconf.c | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index 55208c01ef..f75e674586 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -66,8 +66,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri, } } - if (!(userdir = virGetUserConfigDirectory())) - return -1; + userdir = virGetUserConfigDirectory(); *path = g_strdup_printf("%s/auth.conf", userdir); diff --git a/src/util/virconf.c b/src/util/virconf.c index dce84cabb7..cd18ea61e6 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1507,8 +1507,6 @@ virConfLoadConfigPath(const char *name) path = g_strdup_printf("%s/libvirt/%s", SYSCONFDIR, name); } else { char *userdir = virGetUserConfigDirectory(); - if (!userdir) - return NULL; path = g_strdup_printf("%s/%s", userdir, name); VIR_FREE(userdir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:12AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virauth.c | 3 +-- src/util/virconf.c | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/util/virconf.c b/src/util/virconf.c index dce84cabb7..cd18ea61e6 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1507,8 +1507,6 @@ virConfLoadConfigPath(const char *name) path = g_strdup_printf("%s/libvirt/%s", SYSCONFDIR, name); } else { char *userdir = virGetUserConfigDirectory(); - if (!userdir) - return NULL;
Missed g_autofree opportunity
path = g_strdup_printf("%s/%s", userdir, name); VIR_FREE(userdir);
Whether you squash it in or not: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 580a5e6f15..71078dfbd6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -278,7 +278,7 @@ storageStateInitialize(bool privileged, } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); - if (!(configdir && rundir)) + if (!rundir) goto error; driver->configDir = g_strdup_printf("%s/storage", configdir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:13AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 580a5e6f15..71078dfbd6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -278,7 +278,7 @@ storageStateInitialize(bool privileged, } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); - if (!(configdir && rundir)) + if (!rundir) goto error;
Looking at virGetUserRuntimeDirectory, that one should always return as well, so you can delete them both (adjusting the commit message) Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Dec 19, 2019 at 5:51 PM Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:13AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 580a5e6f15..71078dfbd6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -278,7 +278,7 @@ storageStateInitialize(bool privileged, } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); - if (!(configdir && rundir)) + if (!rundir) goto error;
Looking at virGetUserRuntimeDirectory, that one should always return as well, so you can delete them both (adjusting the commit message)
It's done later in the series, when I change all the virGetUserRuntimeDirectory() cases. Best Regards, -- Fabiano Fidêncio
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/secret/secret_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 93b4256450..cdc4b7dcf9 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -475,8 +475,7 @@ secretStateInitialize(bool privileged, g_autofree char *rundir = NULL; g_autofree char *cfgdir = NULL; - if (!(cfgdir = virGetUserConfigDirectory())) - goto error; + cfgdir = virGetUserConfigDirectory(); driver->configDir = g_strdup_printf("%s/secrets/", cfgdir); if (!(rundir = virGetUserRuntimeDirectory())) -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:14AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/secret/secret_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- tools/vsh.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index bbb6227130..b982aeb359 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2913,11 +2913,6 @@ vshReadlineInit(vshControl *ctl) */ userdir = virGetUserCacheDirectory(); - if (userdir == NULL) { - vshError(ctl, "%s", _("Could not determine home directory")); - goto cleanup; - } - ctl->historydir = g_strdup_printf("%s/%s", userdir, ctl->name); ctl->historyfile = g_strdup_printf("%s/history", ctl->historydir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:15AM +0100, Fabiano Fidêncio wrote:
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- tools/vsh.c | 5 ----- 1 file changed, 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This also fixes a cacheDir's leak when g_mkstep_full() fails. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_common.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 5957ae52c4..586937fa19 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -7341,8 +7341,8 @@ vboxDomainScreenshot(virDomainPtr dom, vboxIID iid; IMachine *machine = NULL; nsresult rc; - char *tmp; - char *cacheDir; + g_autofree char *tmp = NULL; + g_autofree char *cacheDir = NULL; int tmp_fd = -1; unsigned int max_screen; bool privileged = geteuid() == 0; @@ -7383,7 +7383,6 @@ vboxDomainScreenshot(virDomainPtr dom, if ((tmp_fd = g_mkstemp_full(tmp, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) { virReportSystemError(errno, _("g_mkstemp(\"%s\") failed"), tmp); - VIR_FREE(tmp); VBOX_RELEASE(machine); return NULL; } @@ -7454,8 +7453,6 @@ vboxDomainScreenshot(virDomainPtr dom, VIR_FORCE_CLOSE(tmp_fd); unlink(tmp); - VIR_FREE(tmp); - VIR_FREE(cacheDir); VBOX_RELEASE(machine); vboxIIDUnalloc(&iid); return ret; -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:16AM +0100, Fabiano Fidêncio wrote:
This also fixes a cacheDir's leak when g_mkstep_full() fails.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_common.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_common.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 586937fa19..4493fe8582 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -7374,9 +7374,8 @@ vboxDomainScreenshot(virDomainPtr dom, if (privileged) { cacheDir = g_strdup_printf("%s/cache/libvirt", LOCALSTATEDIR); - } else if (!(cacheDir = virGetUserCacheDirectory())) { - VBOX_RELEASE(machine); - return NULL; + } else { + cacheDir = virGetUserCacheDirectory(); } tmp = g_strdup_printf("%s/vbox.screendump.XXXXXX", cacheDir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:17AM +0100, Fabiano Fidêncio wrote:
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/vbox/vbox_common.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virlog.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index d45e2dd316..10639d7328 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -173,8 +173,7 @@ virLogSetDefaultOutputToJournald(void) static int virLogSetDefaultOutputToFile(const char *binary, bool privileged) { - int ret = -1; - char *logdir = NULL; + g_autofree char *logdir = NULL; mode_t old_umask; if (privileged) { @@ -182,12 +181,12 @@ virLogSetDefaultOutputToFile(const char *binary, bool privileged) virLogDefaultPriority, LOCALSTATEDIR, binary); } else { if (!(logdir = virGetUserCacheDirectory())) - goto cleanup; + return -1; old_umask = umask(077); if (virFileMakePath(logdir) < 0) { umask(old_umask); - goto cleanup; + return -1; } umask(old_umask); @@ -195,10 +194,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool privileged) virLogDefaultPriority, logdir, binary); } - ret = 0; - cleanup: - VIR_FREE(logdir); - return ret; + return 0; } -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:18AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virlog.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virlog.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 10639d7328..6bae56e2e3 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -180,8 +180,7 @@ virLogSetDefaultOutputToFile(const char *binary, bool privileged) virLogDefaultOutput = g_strdup_printf("%d:file:%s/log/libvirt/%s.log", virLogDefaultPriority, LOCALSTATEDIR, binary); } else { - if (!(logdir = virGetUserCacheDirectory())) - return -1; + logdir = virGetUserCacheDirectory(); old_umask = umask(077); if (virFileMakePath(logdir) < 0) { -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:19AM +0100, Fabiano Fidêncio wrote:
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virlog.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_conf.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 389ac55bee..634c65095e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -165,8 +165,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) g_autofree char *cachedir = NULL; cachedir = virGetUserCacheDirectory(); - if (!cachedir) - return NULL; cfg->logDir = g_strdup_printf("%s/qemu/log", cachedir); cfg->swtpmLogDir = g_strdup_printf("%s/qemu/log", cachedir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:20AM +0100, Fabiano Fidêncio wrote:
virGetUserCacheDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_conf.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 40e5fa35e2..eba8b865d1 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -455,11 +455,9 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, knownhosts = g_strdup(knownHostsPath); } else { confdir = virGetUserConfigDirectory(); - if (confdir) { - virBufferAsprintf(&buf, "%s/known_hosts", confdir); - if (!(knownhosts = virBufferContentAndReset(&buf))) - goto no_memory; - } + virBufferAsprintf(&buf, "%s/known_hosts", confdir); + if (!(knownhosts = virBufferContentAndReset(&buf))) + goto no_memory; } if (privkeyPath) { @@ -556,8 +554,7 @@ virNetClientPtr virNetClientNewLibssh(const char *host, knownhosts = g_strdup(knownHostsPath); } else { confdir = virGetUserConfigDirectory(); - if (confdir) - knownhosts = g_strdup_printf("%s/known_hosts", confdir); + knownhosts = g_strdup_printf("%s/known_hosts", confdir); } if (privkeyPath) { -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:21AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 40e5fa35e2..eba8b865d1 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -455,11 +455,9 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, knownhosts = g_strdup(knownHostsPath); } else { confdir = virGetUserConfigDirectory(); - if (confdir) { - virBufferAsprintf(&buf, "%s/known_hosts", confdir); - if (!(knownhosts = virBufferContentAndReset(&buf))) - goto no_memory; - } + virBufferAsprintf(&buf, "%s/known_hosts", confdir); + if (!(knownhosts = virBufferContentAndReset(&buf))) + goto no_memory;
no_memory seems suspicious in the times of abort()
}
if (privkeyPath) {
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Dec 19, 2019 at 8:23 PM Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:21AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetclient.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 40e5fa35e2..eba8b865d1 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -455,11 +455,9 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, knownhosts = g_strdup(knownHostsPath); } else { confdir = virGetUserConfigDirectory(); - if (confdir) { - virBufferAsprintf(&buf, "%s/known_hosts", confdir); - if (!(knownhosts = virBufferContentAndReset(&buf))) - goto no_memory; - } + virBufferAsprintf(&buf, "%s/known_hosts", confdir); + if (!(knownhosts = virBufferContentAndReset(&buf))) + goto no_memory;
no_memory seems suspicious in the times of abort()
Indeed. But that's material for another series. Best Regards, -- Fabiano Fidêncio
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/remote/remote_daemon_config.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c index ae6b491686..84e331474b 100644 --- a/src/remote/remote_daemon_config.c +++ b/src/remote/remote_daemon_config.c @@ -81,17 +81,13 @@ daemonConfigFilePath(bool privileged, char **configfile) } else { char *configdir = NULL; - if (!(configdir = virGetUserConfigDirectory())) - goto error; + configdir = virGetUserConfigDirectory(); *configfile = g_strdup_printf("%s/%s.conf", configdir, DAEMON_NAME); VIR_FREE(configdir); } return 0; - - error: - return -1; } struct daemonConfig* -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:22AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/remote/remote_daemon_config.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c index ae6b491686..84e331474b 100644 --- a/src/remote/remote_daemon_config.c +++ b/src/remote/remote_daemon_config.c @@ -81,17 +81,13 @@ daemonConfigFilePath(bool privileged, char **configfile) } else { char *configdir = NULL;
- if (!(configdir = virGetUserConfigDirectory())) - goto error; + configdir = virGetUserConfigDirectory();
*configfile = g_strdup_printf("%s/%s.conf", configdir, DAEMON_NAME); VIR_FREE(configdir);
Another g_autofree opportunity
}
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 634c65095e..aa96d50e41 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -177,8 +177,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); - if (!(cfg->configBaseDir = virGetUserConfigDirectory())) - return NULL; + cfg->configBaseDir = virGetUserConfigDirectory(); cfg->libDir = g_strdup_printf("%s/qemu/lib", cfg->configBaseDir); cfg->saveDir = g_strdup_printf("%s/qemu/save", cfg->configBaseDir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:23AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e360645969..98aa530715 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -741,7 +741,7 @@ networkStateInitialize(bool privileged, } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); - if (!(configdir && rundir)) + if (!rundir) goto error; network_driver->networkConfigDir = g_strdup_printf("%s/qemu/networks", configdir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:24AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon_config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/logging/log_daemon_config.c b/src/logging/log_daemon_config.c index 0cf9729e7f..ab42921140 100644 --- a/src/logging/log_daemon_config.c +++ b/src/logging/log_daemon_config.c @@ -42,13 +42,12 @@ virLogDaemonConfigFilePath(bool privileged, char **configfile) if (privileged) { *configfile = g_strdup(SYSCONFDIR "/libvirt/virtlogd.conf"); } else { - char *configdir = NULL; + g_autofree char *configdir = NULL; if (!(configdir = virGetUserConfigDirectory())) goto error; *configfile = g_strdup_printf("%s/virtlogd.conf", configdir); - VIR_FREE(configdir); } return 0; -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:25AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon_config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon_config.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/logging/log_daemon_config.c b/src/logging/log_daemon_config.c index ab42921140..97f2de90a6 100644 --- a/src/logging/log_daemon_config.c +++ b/src/logging/log_daemon_config.c @@ -44,16 +44,12 @@ virLogDaemonConfigFilePath(bool privileged, char **configfile) } else { g_autofree char *configdir = NULL; - if (!(configdir = virGetUserConfigDirectory())) - goto error; + configdir = virGetUserConfigDirectory(); *configfile = g_strdup_printf("%s/virtlogd.conf", configdir); } return 0; - - error: - return -1; } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:26AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon_config.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon_config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/locking/lock_daemon_config.c b/src/locking/lock_daemon_config.c index d7e13013d7..62df30d9f4 100644 --- a/src/locking/lock_daemon_config.c +++ b/src/locking/lock_daemon_config.c @@ -41,13 +41,12 @@ virLockDaemonConfigFilePath(bool privileged, char **configfile) if (privileged) { *configfile = g_strdup(SYSCONFDIR "/libvirt/virtlockd.conf"); } else { - char *configdir = NULL; + g_autofree char *configdir = NULL; if (!(configdir = virGetUserConfigDirectory())) goto error; *configfile = g_strdup_printf("%s/virtlockd.conf", configdir); - VIR_FREE(configdir); } return 0; -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:27AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon_config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon_config.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/locking/lock_daemon_config.c b/src/locking/lock_daemon_config.c index 62df30d9f4..c67b0a75e1 100644 --- a/src/locking/lock_daemon_config.c +++ b/src/locking/lock_daemon_config.c @@ -43,16 +43,12 @@ virLockDaemonConfigFilePath(bool privileged, char **configfile) } else { g_autofree char *configdir = NULL; - if (!(configdir = virGetUserConfigDirectory())) - goto error; + configdir = virGetUserConfigDirectory(); *configfile = g_strdup_printf("%s/virtlockd.conf", configdir); } return 0; - - error: - return -1; } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:28AM +0100, Fabiano Fidêncio wrote:
virGetUserConfigDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon_config.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virhostdev.c | 3 +-- src/util/virpidfile.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 78e409732a..9b4ea30216 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -182,8 +182,7 @@ virHostdevManagerNew(void) g_autofree char *rundir = NULL; mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) - return NULL; + rundir = virGetUserRuntimeDirectory(); hostdevMgr->stateDir = g_strdup_printf("%s/hostdevmgr", rundir); diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 249515aff2..b08e0d8d52 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -488,8 +488,7 @@ virPidFileConstructPath(bool privileged, } *pidfile = g_strdup_printf("%s/%s.pid", runstatedir, progname); } else { - if (!(rundir = virGetUserRuntimeDirectory())) - return -1; + rundir = virGetUserRuntimeDirectory(); if (virFileMakePathWithMode(rundir, 0700) < 0) { virReportSystemError(errno, -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:29AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/util/virhostdev.c | 3 +-- src/util/virpidfile.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/storage/storage_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 71078dfbd6..a33328db37 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -278,8 +278,6 @@ storageStateInitialize(bool privileged, } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); - if (!rundir) - goto error; driver->configDir = g_strdup_printf("%s/storage", configdir); driver->autostartDir = g_strdup_printf("%s/storage/autostart", configdir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:30AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/storage/storage_driver.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/secret/secret_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index cdc4b7dcf9..096672f114 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -478,8 +478,7 @@ secretStateInitialize(bool privileged, cfgdir = virGetUserConfigDirectory(); driver->configDir = g_strdup_printf("%s/secrets/", cfgdir); - if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + rundir = virGetUserRuntimeDirectory(); driver->stateDir = g_strdup_printf("%s/secrets/run", rundir); } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:31AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/secret/secret_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetsocket.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b98287e6d7..f072afe857 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -684,8 +684,7 @@ int virNetSocketNewConnectUNIX(const char *path, goto cleanup; } - if (!(rundir = virGetUserRuntimeDirectory())) - goto cleanup; + rundir = virGetUserRuntimeDirectory(); if (virFileMakePathWithMode(rundir, 0700) < 0) { virReportSystemError(errno, -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:32AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/rpc/virnetsocket.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/remote/remote_daemon.c | 8 +------- src/remote/remote_driver.c | 3 +-- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index b400b1dd10..5893492875 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -245,8 +245,7 @@ daemonUnixSocketPaths(struct daemonConfig *config, } else { mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) - goto cleanup; + rundir = virGetUserRuntimeDirectory(); old_umask = umask(077); if (virFileMakePath(rundir) < 0) { @@ -1208,11 +1207,6 @@ int main(int argc, char **argv) { run_dir = g_strdup(RUNSTATEDIR "/libvirt"); } else { run_dir = virGetUserRuntimeDirectory(); - - if (!run_dir) { - VIR_ERROR(_("Can't determine user directory")); - goto cleanup; - } } if (privileged) old_umask = umask(022); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e684fcba09..c11f73ab4d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -779,8 +779,7 @@ remoteGetUNIXSocketHelper(remoteDriverTransport transport, remoteDriverTransportTypeToString(transport)); return NULL; } - if (!(userdir = virGetUserRuntimeDirectory())) - return NULL; + userdir = virGetUserRuntimeDirectory(); sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix); } else { -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:33AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/remote/remote_daemon.c | 8 +------- src/remote/remote_driver.c | 3 +-- 2 files changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_conf.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index aa96d50e41..c07a844dfc 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -171,8 +171,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->cacheDir = g_strdup_printf("%s/qemu/cache", cachedir); rundir = virGetUserRuntimeDirectory(); - if (!rundir) - return NULL; cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:34AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/qemu/qemu_conf.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/node_device/node_device_hal.c | 3 +-- src/node_device/node_device_udev.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index b40f93df46..cf12854fe9 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -618,8 +618,7 @@ nodeStateInitialize(bool privileged G_GNUC_UNUSED, } else { g_autofree char *rundir = NULL; - if (!(rundir = virGetUserRuntimeDirectory())) - goto failure; + rundir = virGetUserRuntimeDirectory(); driver->stateDir = g_strdup_printf("%s/nodedev/run", rundir); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index cae00dc9dc..eedcd123a3 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1806,8 +1806,7 @@ nodeStateInitialize(bool privileged, } else { g_autofree char *rundir = NULL; - if (!(rundir = virGetUserRuntimeDirectory())) - goto cleanup; + rundir = virGetUserRuntimeDirectory(); driver->stateDir = g_strdup_printf("%s/nodedev/run", rundir); } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:35AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/node_device/node_device_hal.c | 3 +-- src/node_device/node_device_udev.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/network/bridge_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 98aa530715..c9c45df758 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -741,8 +741,6 @@ networkStateInitialize(bool privileged, } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); - if (!rundir) - goto error; network_driver->networkConfigDir = g_strdup_printf("%s/qemu/networks", configdir); network_driver->networkAutostartDir = g_strdup_printf("%s/qemu/networks/autostart", configdir); -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:36AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/network/bridge_driver.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_manager.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/logging/log_manager.c b/src/logging/log_manager.c index e191093272..7a4f036240 100644 --- a/src/logging/log_manager.c +++ b/src/logging/log_manager.c @@ -47,14 +47,12 @@ virLogManagerDaemonPath(bool privileged) if (privileged) { path = g_strdup(RUNSTATEDIR "/libvirt/virtlogd-sock"); } else { - char *rundir = NULL; + g_autofree char *rundir = NULL; if (!(rundir = virGetUserRuntimeDirectory())) return NULL; path = g_strdup_printf("%s/virtlogd-sock", rundir); - - VIR_FREE(rundir); } return path; } -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:37AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_manager.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Together with the change, let's also simplify the function and get rid of the goto. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 268d3c62b9..d41d9caba9 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -391,29 +391,23 @@ virLogDaemonUnixSocketPaths(bool privileged, *sockfile = g_strdup(RUNSTATEDIR "/libvirt/virtlogd-sock"); *adminSockfile = g_strdup(RUNSTATEDIR "/libvirt/virtlogd-admin-sock"); } else { - char *rundir = NULL; + g_autofree char *rundir = NULL; mode_t old_umask; if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + return -1; old_umask = umask(077); if (virFileMakePath(rundir) < 0) { umask(old_umask); - VIR_FREE(rundir); - goto error; + return -1; } umask(old_umask); *sockfile = g_strdup_printf("%s/virtlogd-sock", rundir); *adminSockfile = g_strdup_printf("%s/virtlogd-admin-sock", rundir); - - VIR_FREE(rundir); } return 0; - - error: - return -1; } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:38AM +0100, Fabiano Fidêncio wrote:
Together with the change, let's also simplify the function and get rid of the goto.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Together with the change, let's also simplify the function and get rid of the goto. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index d41d9caba9..3fee2b3b57 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -612,29 +612,23 @@ virLogDaemonExecRestartStatePath(bool privileged, if (privileged) { *state_file = g_strdup(RUNSTATEDIR "/virtlogd-restart-exec.json"); } else { - char *rundir = NULL; + g_autofree char *rundir = NULL; mode_t old_umask; if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + return -1; old_umask = umask(077); if (virFileMakePath(rundir) < 0) { umask(old_umask); - VIR_FREE(rundir); - goto error; + return -1; } umask(old_umask); *state_file = g_strdup_printf("%s/virtlogd-restart-exec.json", rundir); - - VIR_FREE(rundir); } return 0; - - error: - return -1; } -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:39AM +0100, Fabiano Fidêncio wrote:
Together with the change, let's also simplify the function and get rid of the goto.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon.c | 11 +++-------- src/logging/log_manager.c | 3 +-- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index 3fee2b3b57..488f3b459d 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -394,8 +394,7 @@ virLogDaemonUnixSocketPaths(bool privileged, g_autofree char *rundir = NULL; mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) - return -1; + rundir = virGetUserRuntimeDirectory(); old_umask = umask(077); if (virFileMakePath(rundir) < 0) { @@ -615,8 +614,7 @@ virLogDaemonExecRestartStatePath(bool privileged, g_autofree char *rundir = NULL; mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) - return -1; + rundir = virGetUserRuntimeDirectory(); old_umask = umask(077); if (virFileMakePath(rundir) < 0) { @@ -992,10 +990,7 @@ int main(int argc, char **argv) { if (privileged) { run_dir = g_strdup(RUNSTATEDIR "/libvirt"); } else { - if (!(run_dir = virGetUserRuntimeDirectory())) { - VIR_ERROR(_("Can't determine user directory")); - goto cleanup; - } + run_dir = virGetUserRuntimeDirectory(); } if (privileged) diff --git a/src/logging/log_manager.c b/src/logging/log_manager.c index 7a4f036240..6b66218116 100644 --- a/src/logging/log_manager.c +++ b/src/logging/log_manager.c @@ -49,8 +49,7 @@ virLogManagerDaemonPath(bool privileged) } else { g_autofree char *rundir = NULL; - if (!(rundir = virGetUserRuntimeDirectory())) - return NULL; + rundir = virGetUserRuntimeDirectory(); path = g_strdup_printf("%s/virtlogd-sock", rundir); } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:40AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/logging/log_daemon.c | 11 +++-------- src/logging/log_manager.c | 3 +-- 2 files changed, 4 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_driver_lockd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index e8f0329b05..8ca77e525d 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -122,14 +122,12 @@ static char *virLockManagerLockDaemonPath(bool privileged) if (privileged) { path = g_strdup(RUNSTATEDIR "/libvirt/virtlockd-sock"); } else { - char *rundir = NULL; + g_autofree char *rundir = NULL; if (!(rundir = virGetUserRuntimeDirectory())) return NULL; path = g_strdup_printf("%s/virtlockd-sock", rundir); - - VIR_FREE(rundir); } return path; } -- 2.24.1
s/on/in/ On Thu, Dec 19, 2019 at 11:04:41AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_driver_lockd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Together with the change, let's also simplify the function and get rid of the goto. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 0d12a97231..9bcd36a869 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -449,29 +449,23 @@ virLockDaemonUnixSocketPaths(bool privileged, *sockfile = g_strdup(RUNSTATEDIR "/libvirt/virtlockd-sock"); *adminSockfile = g_strdup(RUNSTATEDIR "/libvirt/virtlockd-admin-sock"); } else { - char *rundir = NULL; + g_autofree char *rundir = NULL; mode_t old_umask; if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + return -1; old_umask = umask(077); if (virFileMakePath(rundir) < 0) { - VIR_FREE(rundir); umask(old_umask); - goto error; + return -1; } umask(old_umask); *sockfile = g_strdup_printf("%s/virtlockd-sock", rundir); *adminSockfile = g_strdup_printf("%s/virtlockd-admin-sock", rundir); - - VIR_FREE(rundir); } return 0; - - error: - return -1; } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:42AM +0100, Fabiano Fidêncio wrote:
Together with the change, let's also simplify the function and get rid of the goto.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Together with the change, let's also simplify the function and get rid of the goto. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 9bcd36a869..7c89adf077 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -820,29 +820,23 @@ virLockDaemonExecRestartStatePath(bool privileged, if (privileged) { *state_file = g_strdup(RUNSTATEDIR "/virtlockd-restart-exec.json"); } else { - char *rundir = NULL; + g_autofree char *rundir = NULL; mode_t old_umask; if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + return -1; old_umask = umask(077); if (virFileMakePath(rundir) < 0) { umask(old_umask); - VIR_FREE(rundir); - goto error; + return -1; } umask(old_umask); *state_file = g_strdup_printf("%s/virtlockd-restart-exec.json", rundir); - - VIR_FREE(rundir); } return 0; - - error: - return -1; } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:43AM +0100, Fabiano Fidêncio wrote:
Together with the change, let's also simplify the function and get rid of the goto.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon.c | 11 +++-------- src/locking/lock_driver_lockd.c | 3 +-- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 7c89adf077..65c38139c4 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -452,8 +452,7 @@ virLockDaemonUnixSocketPaths(bool privileged, g_autofree char *rundir = NULL; mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) - return -1; + rundir = virGetUserRuntimeDirectory(); old_umask = umask(077); if (virFileMakePath(rundir) < 0) { @@ -823,8 +822,7 @@ virLockDaemonExecRestartStatePath(bool privileged, g_autofree char *rundir = NULL; mode_t old_umask; - if (!(rundir = virGetUserRuntimeDirectory())) - return -1; + rundir = virGetUserRuntimeDirectory(); old_umask = umask(077); if (virFileMakePath(rundir) < 0) { @@ -1224,10 +1222,7 @@ int main(int argc, char **argv) { if (privileged) { run_dir = g_strdup(RUNSTATEDIR "/libvirt"); } else { - if (!(run_dir = virGetUserRuntimeDirectory())) { - VIR_ERROR(_("Can't determine user directory")); - goto cleanup; - } + run_dir = virGetUserRuntimeDirectory(); } if (privileged) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 8ca77e525d..339e2f6949 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -124,8 +124,7 @@ static char *virLockManagerLockDaemonPath(bool privileged) } else { g_autofree char *rundir = NULL; - if (!(rundir = virGetUserRuntimeDirectory())) - return NULL; + rundir = virGetUserRuntimeDirectory(); path = g_strdup_printf("%s/virtlockd-sock", rundir); } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:44AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/locking/lock_daemon.c | 11 +++-------- src/locking/lock_driver_lockd.c | 3 +-- 2 files changed, 4 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/interface/interface_backend_netcf.c | 3 +-- src/interface/interface_backend_udev.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index 4f46717cf3..65cb7eae62 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -105,8 +105,7 @@ netcfStateInitialize(bool privileged, } else { g_autofree char *rundir = NULL; - if (!(rundir = virGetUserRuntimeDirectory())) - goto error; + rundir = virGetUserRuntimeDirectory(); driver->stateDir = g_strdup_printf("%s/interface/run", rundir); } diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index 26b1045f8a..7cc098eb33 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -1160,8 +1160,7 @@ udevStateInitialize(bool privileged, } else { g_autofree char *rundir = NULL; - if (!(rundir = virGetUserRuntimeDirectory())) - goto cleanup; + rundir = virGetUserRuntimeDirectory(); driver->stateDir = g_strdup_printf("%s/interface/run", rundir); } -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:45AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/interface/interface_backend_netcf.c | 3 +-- src/interface/interface_backend_udev.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c index f156736d9f..d0c191a56a 100644 --- a/src/admin/libvirt-admin.c +++ b/src/admin/libvirt-admin.c @@ -99,7 +99,7 @@ virAdmInitialize(void) static char * getSocketPath(virURIPtr uri) { - char *rundir = virGetUserRuntimeDirectory(); + g_autofree char *rundir = virGetUserRuntimeDirectory(); char *sock_path = NULL; size_t i = 0; @@ -160,7 +160,6 @@ getSocketPath(virURIPtr uri) } cleanup: - VIR_FREE(rundir); return sock_path; error: -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
s/on/in/ $SUBJECT? This might be the case for other patches as well. One note, I would say it's ok to squash some of the patches from this series together, for example all the g_autofree patches per file for example. Pavel
On Thu, Dec 19, 2019 at 5:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
s/on/in/ $SUBJECT?
This might be the case for other patches as well.
Noted.
One note, I would say it's ok to squash some of the patches from this series together, for example all the g_autofree patches per file for example.
I really thought about that. However, it may be misleading as I'm not touching all the possible conversions to use g_autofree in the other functions.
Pavel
On Thu, Dec 19, 2019 at 05:22:32PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
s/on/in/ $SUBJECT?
This might be the case for other patches as well.
Noted.
One note, I would say it's ok to squash some of the patches from this series together, for example all the g_autofree patches per file for example.
I really thought about that. However, it may be misleading as I'm not touching all the possible conversions to use g_autofree in the other functions.
Well this case is also misleading, since you aren't touching all the possible g_autofree conversions in this functions Jano
Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Dec 19, 2019 at 8:32 PM Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Dec 19, 2019 at 05:22:32PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
s/on/in/ $SUBJECT?
This might be the case for other patches as well.
Noted.
One note, I would say it's ok to squash some of the patches from this series together, for example all the g_autofree patches per file for example.
I really thought about that. However, it may be misleading as I'm not touching all the possible conversions to use g_autofree in the other functions.
Well this case is also misleading, since you aren't touching all the possible g_autofree conversions in this functions
If you're talking specifically about this patch, sock_path is returned. Meaning that we cannot free it when its out of the scope. If you caught some other case, please let me know because as I most likely missed it. Best Regards, -- Fabiano Fidêncio
On 12/19/19 8:33 PM, Ján Tomko wrote:
On Thu, Dec 19, 2019 at 05:22:32PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
s/on/in/ $SUBJECT?
This might be the case for other patches as well.
Noted.
One note, I would say it's ok to squash some of the patches from this series together, for example all the g_autofree patches per file for example.
I really thought about that. However, it may be misleading as I'm not touching all the possible conversions to use g_autofree in the other functions.
Well this case is also misleading, since you aren't touching all the possible g_autofree conversions in this functions
ACK if you squash this in: diff --git i/src/admin/libvirt-admin.c w/src/admin/libvirt-admin.c index dcbb8ca84d..4099a54854 100644 --- i/src/admin/libvirt-admin.c +++ w/src/admin/libvirt-admin.c @@ -100,11 +100,11 @@ static char * getSocketPath(virURIPtr uri) { g_autofree char *rundir = virGetUserRuntimeDirectory(); - char *sock_path = NULL; + g_autofree char *sock_path = NULL; size_t i = 0; if (!uri) - goto cleanup; + return NULL; for (i = 0; i < uri->paramsCount; i++) { @@ -116,7 +116,7 @@ getSocketPath(virURIPtr uri) } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown URI parameter '%s'"), param->name); - goto error; + return NULL; } } @@ -127,7 +127,7 @@ getSocketPath(virURIPtr uri) if (!uri->scheme) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("No URI scheme specified")); - goto error; + return NULL; } if (STREQ(uri->scheme, "libvirtd")) { legacy = true; @@ -135,7 +135,7 @@ getSocketPath(virURIPtr uri) virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported URI scheme '%s'"), uri->scheme); - goto error; + return NULL; } if (legacy) { @@ -152,16 +152,11 @@ getSocketPath(virURIPtr uri) virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid URI path '%s', try '/system'"), NULLSTR_EMPTY(uri->path)); - goto error; + return NULL; } } - cleanup: - return sock_path; - - error: - VIR_FREE(sock_path); - goto cleanup; + return g_steal_pointer(&sock_path); } Michal
On Fri, Dec 20, 2019 at 9:14 AM Michal Prívozník <mprivozn@redhat.com> wrote:
On 12/19/19 8:33 PM, Ján Tomko wrote:
On Thu, Dec 19, 2019 at 05:22:32PM +0100, Fabiano Fidêncio wrote:
On Thu, Dec 19, 2019 at 5:14 PM Pavel Hrdina <phrdina@redhat.com> wrote:
On Thu, Dec 19, 2019 at 11:04:46AM +0100, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
s/on/in/ $SUBJECT?
This might be the case for other patches as well.
Noted.
One note, I would say it's ok to squash some of the patches from this series together, for example all the g_autofree patches per file for example.
I really thought about that. However, it may be misleading as I'm not touching all the possible conversions to use g_autofree in the other functions.
Well this case is also misleading, since you aren't touching all the possible g_autofree conversions in this functions
ACK if you squash this in:
diff --git i/src/admin/libvirt-admin.c w/src/admin/libvirt-admin.c index dcbb8ca84d..4099a54854 100644 --- i/src/admin/libvirt-admin.c +++ w/src/admin/libvirt-admin.c @@ -100,11 +100,11 @@ static char * getSocketPath(virURIPtr uri) { g_autofree char *rundir = virGetUserRuntimeDirectory(); - char *sock_path = NULL; + g_autofree char *sock_path = NULL; size_t i = 0;
if (!uri) - goto cleanup; + return NULL;
for (i = 0; i < uri->paramsCount; i++) { @@ -116,7 +116,7 @@ getSocketPath(virURIPtr uri) } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown URI parameter '%s'"), param->name); - goto error; + return NULL; } }
@@ -127,7 +127,7 @@ getSocketPath(virURIPtr uri) if (!uri->scheme) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("No URI scheme specified")); - goto error; + return NULL; } if (STREQ(uri->scheme, "libvirtd")) { legacy = true; @@ -135,7 +135,7 @@ getSocketPath(virURIPtr uri) virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported URI scheme '%s'"), uri->scheme); - goto error; + return NULL; }
if (legacy) { @@ -152,16 +152,11 @@ getSocketPath(virURIPtr uri) virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid URI path '%s', try '/system'"), NULLSTR_EMPTY(uri->path)); - goto error; + return NULL; } }
- cleanup: - return sock_path; - - error: - VIR_FREE(sock_path); - goto cleanup; + return g_steal_pointer(&sock_path); }
Michal
Thanks, Michal. I really haven't thought about using g_steal_pointer in this case. Taking a "review 101", I'd really expect this to be pointed out as: - If I missed this, it happened most likely because either I'm not aware of this or I didn't see the opportunity of doing such change; - Saying something could be changed but not pointing out what makes the life of **any** developer harder as the developer has to solve a puzzle in order to understand the comment; Please, for the next round, consider (and, at this point, you already know that) that I'm not as smart as the reviewer and point out what has to be changed. :-) Michal went one step further and even provided the diff, it's not needed, but I'd really expect to be pointed to what's wrong during the review. I'll go ahead and push the series. Best Regards, -- Fabiano Fidêncio
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary. Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c index d0c191a56a..dcbb8ca84d 100644 --- a/src/admin/libvirt-admin.c +++ b/src/admin/libvirt-admin.c @@ -147,9 +147,6 @@ getSocketPath(virURIPtr uri) if (STREQ_NULLABLE(uri->path, "/system")) { sock_path = g_strdup_printf(RUNSTATEDIR "/libvirt/%s", sockbase); } else if (STREQ_NULLABLE(uri->path, "/session")) { - if (!rundir) - goto error; - sock_path = g_strdup_printf("%s/%s", rundir, sockbase); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.24.1
On Thu, Dec 19, 2019 at 11:04:47AM +0100, Fabiano Fidêncio wrote:
virGetUserRuntimeDirectory() *never* *ever* returns NULL, making the checks for it completely unnecessary.
Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com> --- src/admin/libvirt-admin.c | 3 --- 1 file changed, 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (5)
-
Daniel Henrique Barboza -
Fabiano Fidêncio -
Ján Tomko -
Michal Prívozník -
Pavel Hrdina