[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 01/13] virprocess: Introduce virProcessRunInFork




On 10/17/18 5:06 AM, Michal Privoznik wrote:
> This new helper can be used to spawn a child process and run
> passed callback from it. This will come handy esp. if the
> callback is not thread safe.
> 
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virprocess.c    | 81 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virprocess.h    | 16 ++++++++
>  3 files changed, 98 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 335210c31d..e2dc85310a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2625,6 +2625,7 @@ virProcessKill;
>  virProcessKillPainfully;
>  virProcessKillPainfullyDelay;
>  virProcessNamespaceAvailable;
> +virProcessRunInFork;
>  virProcessRunInMountNamespace;
>  virProcessSchedPolicyTypeFromString;
>  virProcessSchedPolicyTypeToString;
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 3883c708fc..51b9ccb1bb 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1165,6 +1165,87 @@ virProcessRunInMountNamespace(pid_t pid,
>  }
>  
>  
> +static int
> +virProcessForkHelper(int errfd,

For consistency, virProcessRunInForkHelper

> +                     pid_t pid,

s/pid/parent  (or ppid, IDC)

just to be clear

> +                     virProcessForkCallback cb,
> +                     void *opaque)
> +{
> +    if (cb(pid, opaque) < 0) {
> +        virErrorPtr err = virGetLastError();
> +        if (err) {
> +            size_t len = strlen(err->message) + 1;
> +            ignore_value(safewrite(errfd, err->message, len));
> +        }
> +
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
> +/**
> + * virProcessRunInFork:
> + * @cb: callback to run
> + * @opaque: opaque data to @cb
> + *
> + * Do the fork and run @cb in the child. This can be used when
> + * @cb does something thread unsafe, for instance. However, due
> + * to possible signal propagation @cb should only call async
> + * signal safe functions. On return, the returned value is either
> + * -1 with error message reported if something went bad in the
> + *  parent, if child has died from a signal or if the child
> + *  returned EXIT_CANCELED. Otherwise the returned value is the
> + *  exit status of the child.
> + */
> +int
> +virProcessRunInFork(virProcessForkCallback cb,
> +                    void *opaque)
> +{
> +    int ret = -1;
> +    pid_t child = -1;
> +    pid_t parent = getpid();
> +    int errfd[2] = { -1, -1 };
> +
> +    if (pipe2(errfd, O_CLOEXEC) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Cannot create pipe for child"));
> +        return -1;
> +    }
> +
> +    if ((child = virFork()) < 0)
> +        goto cleanup;
> +
> +    if (child == 0) {
> +        VIR_FORCE_CLOSE(errfd[0]);
> +        ret = virProcessForkHelper(errfd[1], parent, cb, opaque);
> +        VIR_FORCE_CLOSE(errfd[1]);
> +        _exit(ret < 0 ? EXIT_CANCELED : ret);
> +    } else {
> +        int status;
> +        VIR_AUTOFREE(char *) buf = NULL;
> +
> +        VIR_FORCE_CLOSE(errfd[1]);
> +        ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
> +        ret = virProcessWait(child, &status, false);
> +        if (!ret) {

<sigh>

> +            ret = status == EXIT_CANCELED ? -1 : status;
> +            if (ret) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("child reported: %s"),
> +                               NULLSTR(buf));
> +            }

I agree w/ Daniel regarding @status reporting like virCommandWait, it
looks ugly but gets all the information out at least.

> +        }
> +    }
> +
> + cleanup:
> +    VIR_FORCE_CLOSE(errfd[0]);
> +    VIR_FORCE_CLOSE(errfd[1]);
> +    return ret;
> +}
> +
> +
>  #if defined(HAVE_SYS_MOUNT_H) && defined(HAVE_UNSHARE)
>  int
>  virProcessSetupPrivateMountNS(void)
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index 5faa0892fe..1bae8c9212 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -93,6 +93,22 @@ int virProcessRunInMountNamespace(pid_t pid,
>                                    virProcessNamespaceCallback cb,
>                                    void *opaque);
>  
> +/**
> + * virProcessForkCallback:
> + * @pid: parent's pid

It's the "fork's" parent pid. Perhaps would be nice to have ppid or
parent - just because pid is used so many times. It's not that important
though.

> + * @opaque: opaque data
> + *
> + * Callback to run in fork()-ed process.
> + *
> + * Returns: 0 on success,
> + *         -1 on error (treated as EXIT_CANCELED)
> + */
> +typedef int (*virProcessForkCallback)(pid_t pid,
> +                                      void *opaque);
> +
> +int virProcessRunInFork(virProcessForkCallback cb,
> +                        void *opaque);
> +

Since this is defined before virProcessRunInMountNamespace in source,
probably could do the same in this file. It's a type a orderly thing.

and yes, essentially what I was looking to see done from previous
reviews since those paths won't necessarily be NS type runs...

I'm OK with trusting that you can make the necessary comment adjustments
from Daniel's review, but if you feel compelled to post a diff I won't
complain either...

Reviewed-by: John Ferlan <jferlan redhat com>

John

>  int virProcessSetupPrivateMountNS(void);
>  
>  int virProcessSetScheduler(pid_t pid,
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]