From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 1565C5A0082 for ; Thu, 16 Feb 2023 17:38:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676565531; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=llWP4XZ7lViBFo6b6Iui2iRiyOpLlwkJRCNNJ2AvCZw=; b=gH8+/rJQfdHPLWtANF+j+nOQWYwFaAlzK1jS3Ulg/fu0w22rQEiBzC6eHz+4JNEy+K9U00 x0yW1EcMoW1E+w3HhbBZLj3UzI2CsPYvSnCEJD4xlUPq3V01PqMpESwy93tGvnwG4ksIDC YDEu2F/M5RQrUJe7PxA/Yj8WOXXKi34= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-197-rbtSw7uEOQO7fYA0cKeevg-1; Thu, 16 Feb 2023 11:38:49 -0500 X-MC-Unique: rbtSw7uEOQO7fYA0cKeevg-1 Received: by mail-ed1-f70.google.com with SMTP id v8-20020a056402348800b004ad062fee5eso2249674edc.17 for ; Thu, 16 Feb 2023 08:38:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=llWP4XZ7lViBFo6b6Iui2iRiyOpLlwkJRCNNJ2AvCZw=; b=Ky8AlQ1QI8ATQvq44Fe3mF4nvth+RBo6r8UsaJ7ChJHxSTAyBNEwWA1kLyBqWG29lD npqWJnx16d5Ozg7VxrDr26+ULaMsE//q7a0jUWZ/d7OORw4IJmmmyG7KSAKUg884s1kA /27ORagYw12bBAUDBsZJrgIg0TQ5qhMqgPazHuQRdff8DvhnR524ARoWNBjpwC2O6foh hnpoM5+f5ZMO3qyKiFpeQvZsVIYW2tQzyTouJaPD0RH9bHT/7DXda2uZsvBsRtHzuVyc JVCcUhwPP3I8B2ZATdAYaN0qp0ApL1Dph7C1uPuMv9W2fprID52wMdD9cEgNcBxu0EYy 44aw== X-Gm-Message-State: AO0yUKUGMKcfAcD9AlXPl1C/icP7++a+0TaP56JQZsfAb4s0JQ2z0rqy dKkfMH8VSi+QyuPUjBPhjUBWqKA0lEpDSQfqhe0ZzjK/GbJf9+7+Y2uiUzhmD0YCsPfsIBL4CCn lVArIJpVdaczu X-Received: by 2002:a05:6402:b3c:b0:4ac:c77f:c14 with SMTP id bo28-20020a0564020b3c00b004acc77f0c14mr7497257edb.29.1676565528126; Thu, 16 Feb 2023 08:38:48 -0800 (PST) X-Google-Smtp-Source: AK7set/XeGJUjXdewJTFC5Bzv1yIBNn0IQZhaiEA5+2js9AyBg3e4RrZa/u9Jl7rrMzrsAlWjqoppQ== X-Received: by 2002:a05:6402:b3c:b0:4ac:c77f:c14 with SMTP id bo28-20020a0564020b3c00b004acc77f0c14mr7497246edb.29.1676565527867; Thu, 16 Feb 2023 08:38:47 -0800 (PST) Received: from [10.43.2.39] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id y42-20020a50bb2d000000b004ab1f97ca2csm1103854ede.60.2023.02.16.08.38.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Feb 2023 08:38:47 -0800 (PST) Message-ID: Date: Thu, 16 Feb 2023 17:38:47 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code To: Stefano Brivio References: <662496e21f07a201eb0ff453306acc286530f1c1.1676554196.git.mprivozn@redhat.com> <20230216170708.6012a7ef@elisabeth> From: =?UTF-8?B?TWljaGFsIFByw612b3puw61r?= In-Reply-To: <20230216170708.6012a7ef@elisabeth> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-MailFrom: mprivozn@redhat.com X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation Message-ID-Hash: 7L7UPW6ZO4WLU6VLSOBJF3NT42MZ3HHA X-Message-ID-Hash: 7L7UPW6ZO4WLU6VLSOBJF3NT42MZ3HHA X-Mailman-Approved-At: Thu, 16 Feb 2023 18:44:41 +0100 CC: libvir-list@redhat.com, passt-dev@passt.top X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 2/16/23 17:07, Stefano Brivio wrote: > On Thu, 16 Feb 2023 14:32:51 +0100 > Michal Privoznik wrote: > >> There are two places where we kill passt: >> >> 1) qemuPasstStop() - called transitively from qemuProcessStop(), >> 2) qemuPasstStart() - after failed start. >> >> Now, the code from 2) lack error preservation (so if there's >> another error during cleanup we might overwrite the original >> error). Therefore, move the internals of qemuPasstStop() into a >> separate function and call it from both places. >> >> Signed-off-by: Michal Privoznik >> --- >> src/qemu/qemu_passt.c | 23 +++++++++++++---------- >> 1 file changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c >> index 881205449b..a4cc9e7166 100644 >> --- a/src/qemu/qemu_passt.c >> +++ b/src/qemu/qemu_passt.c >> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm, >> } >> >> >> -void >> -qemuPasstStop(virDomainObj *vm, >> - virDomainNetDef *net) >> +static void >> +qemuPasstKill(const char *pidfile) > > A minor comment, should you respin: I think it should be made clear that > this is not the expected/normal way in which passt will terminate -- > here or in the next patch. Removing the PID file is nice, but that's > (usually) about it. I can adjust the commit message. > >> { >> - g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net); >> virErrorPtr orig_err; >> >> virErrorPreserveLast(&orig_err); >> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm, >> } >> >> >> +void >> +qemuPasstStop(virDomainObj *vm, >> + virDomainNetDef *net) >> +{ >> + g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net); >> + >> + qemuPasstKill(pidfile); >> +} >> + >> + >> int >> qemuPasstSetupCgroup(virDomainObj *vm, >> virDomainNetDef *net, >> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm, >> g_autofree char *errbuf = NULL; >> char macaddr[VIR_MAC_STRING_BUFLEN]; >> size_t i; >> - pid_t pid = (pid_t) -1; >> int exitstatus = 0; >> int cmdret = 0; >> >> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm, >> return 0; >> >> error: >> - ignore_value(virPidFileReadPathIfLocked(pidfile, &pid)); >> - if (pid != -1) >> - virProcessKillPainfully(pid, true); >> - unlink(pidfile); >> - >> + qemuPasstKill(pidfile); > > ...what takes care of terminating passt in case qemu doesn't start, now? > The fact that the process is in the cgroup, right? No, it's qemuPasstKill(). Starting a guest is done in qemuProcessStart(). In here, qemuProcessLaunch() -> qemuExtDevicesStart() -> qemuPasstStart() is called. Now, in the top most parent (qemuProcessStart()) - you can see the 'stop' label in which qemuProcessStop() -> qemuExtDevicesStop() -> qemuPasstStop() is called. We could go the extra step and use either virCgroupKillRecursive() or virCgroupKillPainfully() to kill all processes within the CGroup. BUT, the driver runs on different OSes than Linux (e.g. FreeBSD) or on Linux with no CGroups. Michal