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 217125A005E for ; Wed, 15 Feb 2023 08:23:01 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676445780; 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=wL5jlENWLYfsHNgmN6Agp1Etf1Y0Fw/WNCtwdiXii8I=; b=jWB3VBMO+OzVu9StqyRMEbfyu+BGgWPpMAna6OZBhuS8oPSGjHlJWHNuIXSUJH5TXSEGrv z0dq1zruIOwkrcKCWe0xE6rOEcrSjfHlor8FptyQGvCUThLn+/zpG4Xs82U0KsYi4AvcVd HkfYoFQxQv9MZPPMbjs4K4bRp6htpgA= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-146-9NfwDA2SPHaGgwR49YefAQ-1; Wed, 15 Feb 2023 02:22:58 -0500 X-MC-Unique: 9NfwDA2SPHaGgwR49YefAQ-1 Received: by mail-qk1-f199.google.com with SMTP id q13-20020a37f70d000000b007283b33bfb3so10869528qkj.4 for ; Tue, 14 Feb 2023 23:22:58 -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:organization: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=wL5jlENWLYfsHNgmN6Agp1Etf1Y0Fw/WNCtwdiXii8I=; b=zNfLM5qRmn2J4NPj6M52kPoSaCOlvCipAyWdiWIGlYpcKVKTMNWYSOHzyG0P5vRA8p HR5CA6QCJmFWOWKzQjSdWyZRjqWu7g0ESUmuZWDGTTeI7U6oiWw2J+6LsEaIEzqxjXU2 uvsKbYN3QrqaDWzZwAVMYnYiygJ6Mqtb8MmSKksoEmy2Vqt339jHsCIFeUCij+HAcF0d eTLbgY2ZamjuA6FLnfZvfYp78k6S4u1SFknGNhtEjKH3LcZuFJGxU8UyXCKb09vhYUpB /yx0Qf7rsbTK+jVWIZG+VILJLnse1k1UX9/dUlsS13sY7oD3XrCPAswLKSZ9DtlB4fvR z0mg== X-Gm-Message-State: AO0yUKUDmkCu+8ng1M079SzaXOH81HJTVt2jPUaUJOf3wxxI14fiw/By RnRhfE5xo0f7INl1cJ57GcYR/8S0qlNKzd2oCWpmD431Pz10eD++LuOu+lmy5eTHuL2TrXowBv7 B4YefoLNqDiYI X-Received: by 2002:ac8:4e43:0:b0:3b6:9b37:e03c with SMTP id e3-20020ac84e43000000b003b69b37e03cmr2186367qtw.7.1676445778255; Tue, 14 Feb 2023 23:22:58 -0800 (PST) X-Google-Smtp-Source: AK7set8wPQx5Z51ZHMPpqQJs/eLjz/c9jBvB1WSaSiHNy4f7HIsO485ZVAIzWf96ZMfXtoJttcLlmg== X-Received: by 2002:ac8:4e43:0:b0:3b6:9b37:e03c with SMTP id e3-20020ac84e43000000b003b69b37e03cmr2186347qtw.7.1676445777988; Tue, 14 Feb 2023 23:22:57 -0800 (PST) Received: from ?IPV6:2600:8805:3a00:3:3b4f:6d3c:92c4:a5c7? ([2600:8805:3a00:3:3b4f:6d3c:92c4:a5c7]) by smtp.gmail.com with ESMTPSA id x66-20020a379545000000b0070d11191e91sm13520719qkd.44.2023.02.14.23.22.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Feb 2023 23:22:57 -0800 (PST) Message-ID: Date: Wed, 15 Feb 2023 02:22:57 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH 2/4] qemu_extdevice: Make qemuExtDevicesHasDevice() check def->nets To: libvir-list@redhat.com References: <446dd5c17d7c3aa302f30253edc33fa8788100fa.1676374699.git.mprivozn@redhat.com> From: Laine Stump Organization: Red Hat In-Reply-To: <446dd5c17d7c3aa302f30253edc33fa8788100fa.1676374699.git.mprivozn@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Message-ID-Hash: JN7NQW37LN4762CZQCHPDRK5NLVBWYDF X-Message-ID-Hash: JN7NQW37LN4762CZQCHPDRK5NLVBWYDF X-MailFrom: laine@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: sbrivio@redhat.com, passt-dev@passt.top, Michal Privoznik 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/14/23 6:51 AM, Michal Privoznik wrote: > We can have external helper processes running for domain > too (e.g. slirp or passt). But this is not reflected > in qemuExtDevicesHasDevice() which simply ignores these. The slirp-helper patches missed adding the check in this (oddly-named) function (even while adding in the correct hunk to qemuExtDevicesSetupCroup()) probably because it wasn't really obvious without reading/interpreting/understanding all the code in two separate files that it was needed; my passt patches missed adding the check in this function because I was following the pattern of what was done for slirp, and slirp hadn't touched this function (nor had it touched the function that calls both of these functions, qemuSetupCgroupForExtDevices(), which is in another file). It's reasonable to think that some future person may also not notice qemuExtDevicesHasDevice(), and believe that they only need to modify qemuExtDevicesSetupCgroup(). Anyway, my point is that I think this could be avoided by adding a comment in qemuExtDevicesSetupCgroup() that points out it is only called if qemuExtDevicesHasDevice() returns true, and so any addition to qemuExtDevicesSetupCgroup() should have a corresponding addition to qemuExtDevicesHasDevice(). It's too late at night / early in the morning for my brain to compose a concise sentence to this effect, but it would make me happy if you added one before pushing. > > Signed-off-by: Michal Privoznik Reviewed-by: Laine Stump > --- > src/qemu/qemu_extdevice.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c > index fdefe59215..47e97f3565 100644 > --- a/src/qemu/qemu_extdevice.c > +++ b/src/qemu/qemu_extdevice.c > @@ -296,6 +296,17 @@ qemuExtDevicesHasDevice(virDomainDef *def) > return true; > } > > + for (i = 0; i < def->nnets; i++) { > + virDomainNetDef *net = def->nets[i]; > + > + if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) > + return true; > + > + if (net->type == VIR_DOMAIN_NET_TYPE_USER && > + net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) > + return true; > + } > + > for (i = 0; i < def->ntpms; i++) { > if (def->tpms[i]->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) > return true;