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.133.124]) by passt.top (Postfix) with ESMTP id 7AE695A005E for ; Thu, 9 Feb 2023 11:09:29 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675937368; 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=HHCzjZa3cdrtc4mXv4BYW6wOlvAozyeF8K3Te/VfipU=; b=PpDNiC6ALRxX/yBLIcLMe8nJfuDBzU+1E48X9MqoqSf+M4RF0dMHuf4Vdn+a+fa+0sRifg o3ECOjMMojleO8FHCS/2hJxMAsrEYbqjFzXO7I1mLA9FKk8oXP8lQPtMp3MHHP/12WhdCX 56lkTweVJWqxBTEDYpujv/wYnctRS4I= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-258-Q52ZjX3DO0KrywlaFDMZKg-1; Thu, 09 Feb 2023 05:09:27 -0500 X-MC-Unique: Q52ZjX3DO0KrywlaFDMZKg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id DF5613C0254D for ; Thu, 9 Feb 2023 10:09:26 +0000 (UTC) Received: from maya.cloud.tilaa.com (ovpn-208-4.brq.redhat.com [10.40.208.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 80DD540C83B6; Thu, 9 Feb 2023 10:09:26 +0000 (UTC) Date: Thu, 9 Feb 2023 11:09:23 +0100 From: Stefano Brivio To: Peter Krempa , Michal =?UTF-8?B?UHLDrXZvem7DrWs=?= Subject: Re: [libvirt PATCH] qemu: allow passt to self-daemonize Message-ID: <20230209110923.5c3a40c9@elisabeth> In-Reply-To: References: <20230208231310.1728051-1-laine@redhat.com> <30ce5926-e7ee-2bdf-33c2-5964032c117e@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: YTYYLYMOLN6VDDA2AUFQWGTROEFAFK3F X-Message-ID-Hash: YTYYLYMOLN6VDDA2AUFQWGTROEFAFK3F X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Laine Stump , 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 Thu, 9 Feb 2023 10:09:38 +0100 Peter Krempa wrote: > On Thu, Feb 09, 2023 at 09:59:54 +0100, Michal Pr=C3=ADvozn=C3=ADk wrote: > > On 2/9/23 09:36, Peter Krempa wrote: =20 > > > On Wed, Feb 08, 2023 at 18:13:10 -0500, Laine Stump wrote: =20 > > >> I initially had the passt process being started in an identical > > >> fashion to the slirp-helper - libvirt was daemonizing the new proces= s > > >> and recording its pid in a pidfile. The problem with this is that, > > >> since it is daemonized immediately, any startup error in passt happe= ns > > >> after the daemonization, and thus isn't seen by libvirt - libvirt > > >> believes that the process has started successfully and continues on > > >> its merry way. The result was that sometimes a guest would be starte= d, > > >> but there would be no passt process for qemu to use for network > > >> traffic. > > >> > > >> Instead, we should be starting passt in the same manner we start > > >> dnsmasq - we just exec it as normal (along with a request that passt > > >> create the pidfile, which is just another option on the passt > > >> commandline) and wait for the child process to exit; passt then has = a > > >> chance to parse its commandline and complete all the setup prior to > > >> daemonizing itself; if it encounters an error and exits with a non-0 > > >> code, libvirt will see the code and know about the failure. We can > > >> then grab the output from stderr, log that so the "user" has some id= ea > > >> of what went wrong, and then fail the guest startup. > > >> > > >> Signed-off-by: Laine Stump > > >> --- > > >> src/qemu/qemu_passt.c | 9 ++++----- > > >> 1 file changed, 4 insertions(+), 5 deletions(-) =20 > > >=20 > > > [..] > > > =20 > > >> if (cmdret < 0 || exitstatus !=3D 0) { > > >> virReportError(VIR_ERR_INTERNAL_ERROR, > > >> - _("Could not start 'passt'. exitstatus: %d")= , exitstatus); > > >> + _("Could not start 'passt': %s"), errbuf); > > >> goto error; > > >> } =20 > > >=20 > > > So the 'passt' binary doesn't do any logging later on during runtime > > > which we'd have to capture into a specific log file? =20 > >=20 > > It does: > >=20 > > -e, --stderr Log to stderr too > > default: log to system logger only if started from a TTY > > -l, --log-file PATH Log (only) to given file > > --log-size BYTES Maximum size of log file > > default: 1 MiB ...yes, but that's already handled earlier in qemuPasstStart(): if (net->backend.logFile) virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL)= ; > > Maybe, we can keep the errfd and let our event loop read from it? But > > that looks like a stretch, unnecessary - what would we do with the erro= r > > if it's reported after guest is started, there's no client connected an= d > > no API running? The best we could do is to relay the error into our > > logs. Which is probably as good as '-l' option then. =20 >=20 > Well, the stdout/err FDs can be passed to virtlogd so that the output is > in the appropriate log file and rotated as needed. I don't think libvirt needs to do that. My assumption is that passt would keep working (and logging) as long as the guest is alive and connected, even if libvirtd and virtlogd terminate -- hence the need for a separate log file. > > BTW: I don't see us passing --stderr. Is that intentional? Maybe I don'= t > > understand the default. =20 >=20 > I don't know the default either, but in this case logging to the system > journal would be not very good as it would be hard for the user to > identify which instance the log belongs to. It's just what it says in the man page: -e, --stderr Log to standard error too. Default is to log to system log= ger only, if started from an interactive terminal, and to both s= ys=E2=80=90 tem logger and standard error otherwise. so you don't need to pass that explicitly. Then, if --log-file is passed: -l, --log-file PATH Log to file PATH, not to standard error, and not to the sys= tem logger. we'll stop logging to standard error and to the system logger, but this only happens (and we should clarify that in the man page, I guess) once options have been parsed, so that any issue with the command line preventing passt from starting is still reported to stderr -- and ends up in 'errbuf', after this patch. For context: Laine just sent a series, for passt: https://archives.passt.top/passt-dev/20230208174838.1680517-1-laine@redha= t.com/ moving the point where we stop logging to stderr a bit later: not once passt is done processing options, but once it daemonises. There would be a few possible error messages (and abort paths) not covered, otherwise. About correlating entries from a system log: if libvirtd logs the PID of any given instance, I think we're all set. --=20 Stefano