[libvirt] [PATCH] libvirtd: create run dir when running at non-root user

[Xu He Jie]

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 at linux.vnet.ibm.com wrote:
>> From: Xu He Jie<xuhj at linux.vnet.ibm.com>
>>
>>
>> Signed-off-by: Xu He Jie<xuhj at 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();
>
>