Now, that our virCommandSetPidFile() is more intelligent we don't
need to rely on the daemon to create and lock the pidfile and use
virCommandSetPidFile() at the same time.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_process.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e8401030a2..0324857913 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2850,7 +2850,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
     g_autoptr(virQEMUDriverConfig) cfg = NULL;
     int errfd = -1;
     g_autofree char *pidfile = NULL;
-    int pidfd = -1;
     g_autofree char *socketPath = NULL;
     pid_t cpid = -1;
     g_autoptr(virCommand) cmd = NULL;
@@ -2869,10 +2868,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
     if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm)))
         goto cleanup;
 
-    /* Just try to acquire. Dummy pid will be replaced later */
-    if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0)
-        goto cleanup;
-
     if (!(socketPath = qemuDomainGetManagedPRSocketPath(priv)))
         goto cleanup;
 
@@ -2887,13 +2882,10 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
 
     if (!(cmd = virCommandNewArgList(cfg->prHelperName,
                                      "-k", socketPath,
-                                     "-f", pidfile,
                                      NULL)))
         goto cleanup;
 
     virCommandDaemonize(cmd);
-    /* We want our virCommand to write child PID into the pidfile
-     * so that we can read it even before exec(). */
     virCommandSetPidFile(cmd, pidfile);
     virCommandSetErrorFD(cmd, &errfd);
 
@@ -2956,7 +2948,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
         if (pidfile)
             unlink(pidfile);
     }
-    VIR_FORCE_CLOSE(pidfd);
     VIR_FORCE_CLOSE(errfd);
     return ret;
 }
-- 
2.24.1

Hi

On Mon, Mar 23, 2020 at 5:16 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> Now, that our virCommandSetPidFile() is more intelligent we don't
> need to rely on the daemon to create and lock the pidfile and use
> virCommandSetPidFile() at the same time.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Nice, but doesn't this also fix a temporary regression introduced by
previous commit, now that pidfile is locked?

> ---
>  src/qemu/qemu_process.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e8401030a2..0324857913 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2850,7 +2850,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
>      g_autoptr(virQEMUDriverConfig) cfg = NULL;
>      int errfd = -1;
>      g_autofree char *pidfile = NULL;
> -    int pidfd = -1;
>      g_autofree char *socketPath = NULL;
>      pid_t cpid = -1;
>      g_autoptr(virCommand) cmd = NULL;
> @@ -2869,10 +2868,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
>      if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm)))
>          goto cleanup;
>
> -    /* Just try to acquire. Dummy pid will be replaced later */
> -    if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0)
> -        goto cleanup;
> -
>      if (!(socketPath = qemuDomainGetManagedPRSocketPath(priv)))
>          goto cleanup;
>
> @@ -2887,13 +2882,10 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
>
>      if (!(cmd = virCommandNewArgList(cfg->prHelperName,
>                                       "-k", socketPath,
> -                                     "-f", pidfile,
>                                       NULL)))
>          goto cleanup;
>
>      virCommandDaemonize(cmd);
> -    /* We want our virCommand to write child PID into the pidfile
> -     * so that we can read it even before exec(). */
>      virCommandSetPidFile(cmd, pidfile);
>      virCommandSetErrorFD(cmd, &errfd);
>
> @@ -2956,7 +2948,6 @@ qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
>          if (pidfile)
>              unlink(pidfile);
>      }
> -    VIR_FORCE_CLOSE(pidfd);
>      VIR_FORCE_CLOSE(errfd);
>      return ret;
>  }
> --
> 2.24.1
>


-- 
Marc-André Lureau

On 23. 3. 2020 17:36, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 23, 2020 at 5:16 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> Now, that our virCommandSetPidFile() is more intelligent we don't
>> need to rely on the daemon to create and lock the pidfile and use
>> virCommandSetPidFile() at the same time.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Nice, but doesn't this also fix a temporary regression introduced by
> previous commit, now that pidfile is locked?

Yeah, I haven't found a way to do this regression free.

Michal

Hi

On Mon, Mar 23, 2020 at 6:48 PM Michal Prívozník <mprivozn@redhat.com> wrote:
>
> On 23. 3. 2020 17:36, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Mar 23, 2020 at 5:16 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>
> >> Now, that our virCommandSetPidFile() is more intelligent we don't
> >> need to rely on the daemon to create and lock the pidfile and use
> >> virCommandSetPidFile() at the same time.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >
> > Nice, but doesn't this also fix a temporary regression introduced by
> > previous commit, now that pidfile is locked?
>
> Yeah, I haven't found a way to do this regression free.

Either squash them,

Or add a fat warning on the previous commit, and mention that you fix
it here too.

With that,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau