[libvirt] [PATCH] libvirtd: create run dir when running at non-root user
From: Xu He Jie <xuhj@linux.vnet.ibm.com> Signed-off-by: Xu He Jie <xuhj@linux.vnet.ibm.com> When libvirtd is running at non-root user, it won't create ${HOME}/.libvirt. It will show error message: 17:44:16.838: 7035: error : virPidFileAcquirePath:322 : Failed to open pid file --- daemon/libvirtd.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 files changed, 32 insertions(+), 14 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 423c3d7..e0004c7 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1249,6 +1249,7 @@ int main(int argc, char **argv) { bool privileged = geteuid() == 0 ? true : false; bool implicit_conf = false; bool use_polkit_dbus; + char *run_dir = NULL; struct option opts[] = { { "verbose", no_argument, &verbose, 1}, @@ -1403,21 +1404,35 @@ int main(int argc, char **argv) { /* Ensure the rundir exists (on tmpfs on some systems) */ if (privileged) { - const char *rundir = LOCALSTATEDIR "/run/libvirt"; - mode_t old_umask; - - old_umask = umask(022); - if (mkdir (rundir, 0755)) { - if (errno != EEXIST) { - char ebuf[1024]; - VIR_ERROR(_("unable to create rundir %s: %s"), rundir, - virStrerror(errno, ebuf, sizeof(ebuf))); - ret = VIR_DAEMON_ERR_RUNDIR; - umask(old_umask); - goto cleanup; - } + run_dir = strdup(LOCALSTATEDIR "/run/libvirt"); + if (!run_dir) { + VIR_ERROR(_("Can't allocate memory")); + goto cleanup; + } + } + else { + char *user_dir = NULL; + + if (!(user_dir = virGetUserDirectory(geteuid()))) { + VIR_ERROR(_("Can't determine user directory")); + goto cleanup; + } + + if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir) < 0) { + VIR_ERROR(_("Can't allocate memory")); + VIR_FREE(user_dir); + goto cleanup; } - umask(old_umask); + + VIR_FREE(user_dir); + } + + if (virFileMakePath(run_dir) < 0) { + char ebuf[1024]; + VIR_ERROR(_("unable to create rundir %s: %s"), run_dir, + virStrerror(errno, ebuf, sizeof(ebuf))); + ret = VIR_DAEMON_ERR_RUNDIR; + goto cleanup; } /* Try to claim the pidfile, exiting if we can't */ @@ -1571,6 +1586,9 @@ cleanup: VIR_FREE(sock_file_ro); VIR_FREE(pid_file); VIR_FREE(remote_config_file); + if (run_dir) + VIR_FREE(run_dir); + daemonConfigFree(config); virLogShutdown(); -- 1.7.4.1
Hi, Eric This patch is second version, but I forgot adding patch version at mail's title. Should I send this patch with correct title again? :) And thanks for your patch review and reply. 于 2011年08月29日 10:52, xuhj@linux.vnet.ibm.com 写道:
From: Xu He Jie <xuhj@linux.vnet.ibm.com>
Signed-off-by: Xu He Jie <xuhj@linux.vnet.ibm.com>
When libvirtd is running at non-root user, it won't create ${HOME}/.libvirt.
It will show error message: 17:44:16.838: 7035: error : virPidFileAcquirePath:322 : Failed to open pid file
--- daemon/libvirtd.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 423c3d7..e0004c7 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1249,6 +1249,7 @@ int main(int argc, char **argv) { bool privileged = geteuid() == 0 ? true : false; bool implicit_conf = false; bool use_polkit_dbus; + char *run_dir = NULL;
struct option opts[] = { { "verbose", no_argument, &verbose, 1}, @@ -1403,21 +1404,35 @@ int main(int argc, char **argv) {
/* Ensure the rundir exists (on tmpfs on some systems) */ if (privileged) { - const char *rundir = LOCALSTATEDIR "/run/libvirt"; - mode_t old_umask; - - old_umask = umask(022); - if (mkdir (rundir, 0755)) { - if (errno != EEXIST) { - char ebuf[1024]; - VIR_ERROR(_("unable to create rundir %s: %s"), rundir, - virStrerror(errno, ebuf, sizeof(ebuf))); - ret = VIR_DAEMON_ERR_RUNDIR; - umask(old_umask); - goto cleanup; - } + run_dir = strdup(LOCALSTATEDIR "/run/libvirt"); + if (!run_dir) { + VIR_ERROR(_("Can't allocate memory")); + goto cleanup; + } + } + else { + char *user_dir = NULL; + + if (!(user_dir = virGetUserDirectory(geteuid()))) { + VIR_ERROR(_("Can't determine user directory")); + goto cleanup; + } + + if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir) < 0) { + VIR_ERROR(_("Can't allocate memory")); + VIR_FREE(user_dir); + goto cleanup; } - umask(old_umask); + + VIR_FREE(user_dir); + } + + if (virFileMakePath(run_dir) < 0) { + char ebuf[1024]; + VIR_ERROR(_("unable to create rundir %s: %s"), run_dir, + virStrerror(errno, ebuf, sizeof(ebuf))); + ret = VIR_DAEMON_ERR_RUNDIR; + goto cleanup; }
/* Try to claim the pidfile, exiting if we can't */ @@ -1571,6 +1586,9 @@ cleanup: VIR_FREE(sock_file_ro); VIR_FREE(pid_file); VIR_FREE(remote_config_file); + if (run_dir) + VIR_FREE(run_dir); + daemonConfigFree(config); virLogShutdown();
On 08/28/2011 08:52 PM, xuhj@linux.vnet.ibm.com wrote:
From: Xu He Jie<xuhj@linux.vnet.ibm.com>
Signed-off-by: Xu He Jie<xuhj@linux.vnet.ibm.com>
Usually this line goes on the bottom, not the top, of a commit message.
When libvirtd is running at non-root user, it won't create ${HOME}/.libvirt.
It will show error message: 17:44:16.838: 7035: error : virPidFileAcquirePath:322 : Failed to open pid file
+ run_dir = strdup(LOCALSTATEDIR "/run/libvirt"); + if (!run_dir) { + VIR_ERROR(_("Can't allocate memory"));
virReportOOMError(), not a hand-rolled error reporting.
+ goto cleanup; + } + } + else {
Style: '} else {' with no newline.
+ char *user_dir = NULL; + + if (!(user_dir = virGetUserDirectory(geteuid()))) { + VIR_ERROR(_("Can't determine user directory")); + goto cleanup; + } + + if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir)< 0) { + VIR_ERROR(_("Can't allocate memory"));
We can consolidate OOM checking for both branches by sinking it below the if.
+ VIR_FREE(user_dir); + goto cleanup; } - umask(old_umask);
Hmm, on second thought, virFileMakePath is documented as explicitly using 0777 - umask, but we really do want the new directory to be 0755 if we create it. I think I'll keep the umask modifications intact (if someone can convince me otherwise, especially if we have a use case for 0775 instead of 0777 or 0755, we can change that in a later patch).
+ if (run_dir) + VIR_FREE(run_dir);
Fails 'make syntax-check', due to a useless if before free. ACK with these changes squashed in, so I pushed. I also added you to AUTHORS; let me know if I need to update anything there. diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index e0004c7..7734fff 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1250,6 +1250,7 @@ int main(int argc, char **argv) { bool implicit_conf = false; bool use_polkit_dbus; char *run_dir = NULL; + mode_t old_umask; struct option opts[] = { { "verbose", no_argument, &verbose, 1}, @@ -1405,28 +1406,22 @@ int main(int argc, char **argv) { /* Ensure the rundir exists (on tmpfs on some systems) */ if (privileged) { run_dir = strdup(LOCALSTATEDIR "/run/libvirt"); - if (!run_dir) { - VIR_ERROR(_("Can't allocate memory")); - goto cleanup; - } - } - else { - char *user_dir = NULL; + } else { + char *user_dir = virGetUserDirectory(geteuid()); - if (!(user_dir = virGetUserDirectory(geteuid()))) { + if (!user_dir) { VIR_ERROR(_("Can't determine user directory")); goto cleanup; } - - if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir) < 0) { - VIR_ERROR(_("Can't allocate memory")); - VIR_FREE(user_dir); - goto cleanup; - } - + ignore_value(virAsprintf(&run_dir, "%s/.libvirt/", user_dir)); VIR_FREE(user_dir); } + if (!run_dir) { + virReportOOMError(); + goto cleanup; + } + old_umask = umask(022); if (virFileMakePath(run_dir) < 0) { char ebuf[1024]; VIR_ERROR(_("unable to create rundir %s: %s"), run_dir, @@ -1434,6 +1429,7 @@ int main(int argc, char **argv) { ret = VIR_DAEMON_ERR_RUNDIR; goto cleanup; } + umask(old_umask); /* Try to claim the pidfile, exiting if we can't */ if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) { @@ -1586,8 +1582,7 @@ cleanup: VIR_FREE(sock_file_ro); VIR_FREE(pid_file); VIR_FREE(remote_config_file); - if (run_dir) - VIR_FREE(run_dir); + VIR_FREE(run_dir); daemonConfigFree(config); virLogShutdown(); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Thanks again! It will be better next time. :) On 2011年09月02日 10:37, Eric Blake wrote:
On 08/28/2011 08:52 PM, xuhj@linux.vnet.ibm.com wrote:
From: Xu He Jie<xuhj@linux.vnet.ibm.com>
Signed-off-by: Xu He Jie<xuhj@linux.vnet.ibm.com>
Usually this line goes on the bottom, not the top, of a commit message.
When libvirtd is running at non-root user, it won't create ${HOME}/.libvirt.
It will show error message: 17:44:16.838: 7035: error : virPidFileAcquirePath:322 : Failed to open pid file
+ run_dir = strdup(LOCALSTATEDIR "/run/libvirt"); + if (!run_dir) { + VIR_ERROR(_("Can't allocate memory"));
virReportOOMError(), not a hand-rolled error reporting.
+ goto cleanup; + } + } + else {
Style: '} else {' with no newline.
+ char *user_dir = NULL; + + if (!(user_dir = virGetUserDirectory(geteuid()))) { + VIR_ERROR(_("Can't determine user directory")); + goto cleanup; + } + + if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir)< 0) { + VIR_ERROR(_("Can't allocate memory"));
We can consolidate OOM checking for both branches by sinking it below the if.
+ VIR_FREE(user_dir); + goto cleanup; } - umask(old_umask);
Hmm, on second thought, virFileMakePath is documented as explicitly using 0777 - umask, but we really do want the new directory to be 0755 if we create it. I think I'll keep the umask modifications intact (if someone can convince me otherwise, especially if we have a use case for 0775 instead of 0777 or 0755, we can change that in a later patch).
+ if (run_dir) + VIR_FREE(run_dir);
Fails 'make syntax-check', due to a useless if before free.
ACK with these changes squashed in, so I pushed. I also added you to AUTHORS; let me know if I need to update anything there.
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index e0004c7..7734fff 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1250,6 +1250,7 @@ int main(int argc, char **argv) { bool implicit_conf = false; bool use_polkit_dbus; char *run_dir = NULL; + mode_t old_umask;
struct option opts[] = { { "verbose", no_argument, &verbose, 1}, @@ -1405,28 +1406,22 @@ int main(int argc, char **argv) { /* Ensure the rundir exists (on tmpfs on some systems) */ if (privileged) { run_dir = strdup(LOCALSTATEDIR "/run/libvirt"); - if (!run_dir) { - VIR_ERROR(_("Can't allocate memory")); - goto cleanup; - } - } - else { - char *user_dir = NULL; + } else { + char *user_dir = virGetUserDirectory(geteuid());
- if (!(user_dir = virGetUserDirectory(geteuid()))) { + if (!user_dir) { VIR_ERROR(_("Can't determine user directory")); goto cleanup; } - - if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir) < 0) { - VIR_ERROR(_("Can't allocate memory")); - VIR_FREE(user_dir); - goto cleanup; - } - + ignore_value(virAsprintf(&run_dir, "%s/.libvirt/", user_dir)); VIR_FREE(user_dir); } + if (!run_dir) { + virReportOOMError(); + goto cleanup; + }
+ old_umask = umask(022); if (virFileMakePath(run_dir) < 0) { char ebuf[1024]; VIR_ERROR(_("unable to create rundir %s: %s"), run_dir, @@ -1434,6 +1429,7 @@ int main(int argc, char **argv) { ret = VIR_DAEMON_ERR_RUNDIR; goto cleanup; } + umask(old_umask);
/* Try to claim the pidfile, exiting if we can't */ if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) { @@ -1586,8 +1582,7 @@ cleanup: VIR_FREE(sock_file_ro); VIR_FREE(pid_file); VIR_FREE(remote_config_file); - if (run_dir) - VIR_FREE(run_dir); + VIR_FREE(run_dir);
daemonConfigFree(config); virLogShutdown();
participants (3)
-
Eric Blake -
Xu He Jie -
xuhj＠linux.vnet.ibm.com