From nobody Thu Oct 27 23:44:05 2022 X-Original-To: freebsd-virtualization@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Mz2MC26fjz4gT3J for ; Thu, 27 Oct 2022 23:44:07 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Mz2MC1HGPz3b1B; Thu, 27 Oct 2022 23:44:07 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1666914247; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pVwnrTbxEmHxHX4lMoGN7lS+r1OnRIeN3wwD1XwyrwY=; b=A0jrjbSapeeBXyJ7JUiQBsGN2hDwChzp0oQBWQAfzklc2/KvUe13PAyFoctLWaxjY/8oVu K4ZST2fHNvSwc/9oWS3Tbhwh0DwN1ZxPQOGca38bzczhVk685tuC3AL41Jzy56c55OO3fw IanGQupXuXJWa9Hg0KlVzxHoTPXLZBS+XtOhHMJaAAqVOhW39F6WAZTfM1JMtMiJL7ax03 MSsqsbZ8gEzfTQRbh6ePLBBspY9RkTgR6AzcRwIDgKJHUESHcHs0VniMCi4Mlpk5GC71YB YgsaUd54qrlGCBvfHXUsJtuQ2I+9GCEWUnw122BaZh+lDtpZQJxZdhf7rCdL9A== Received: from [10.0.0.92] (c-98-35-126-114.hsd1.ca.comcast.net [98.35.126.114]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4Mz2MB5NTrz10nh; Thu, 27 Oct 2022 23:44:06 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Thu, 27 Oct 2022 16:44:05 -0700 List-Id: Discussion List-Archive: https://lists.freebsd.org/archives/freebsd-virtualization List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-virtualization@freebsd.org X-BeenThere: freebsd-virtualization@freebsd.org MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: bhyve nvlist constness bug Content-Language: en-US To: Mark Johnston , freebsd-virtualization@freebsd.org References: From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1666914247; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pVwnrTbxEmHxHX4lMoGN7lS+r1OnRIeN3wwD1XwyrwY=; b=ccPDDysckXbMiXeXo55w90QHAFRytFaoWeukew/6MC10agGgcZaBw3jBJEx58HnmJTZ16b MAY0CufaPx2Ki8EA/aBcXmkmQ+c9nRwNKI0rQrhaUTqxC2Oc1ekqLbLtL30LcDasZzcb5X bzUyzJvw8+HZNY2ujupU5PHYYdZDclRv9i8iruNqPhkdU7JqKf8OzB2A8ZSMoMqfQqlZce a2mAn79Zcopp+hkCvqUBddJ/GKH33brZLBEu8h0aPs5xG8cHf7vmt6jvHmiK+fyPH+BaSG 7hg5xilyNcLmNpuCxKAlUdbibo/iOz/MdCdBk8ckawzfqqaQ3zVgnlDmmhQEWw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1666914247; a=rsa-sha256; cv=none; b=qz1JNMps3/orMzaCZK3H5chet2zaaytIY2UtQCZljGPuyMt8FBrdUmxub0peNlvUpQIMqI gnbtaFHyCv/+PrMzWZpeUbYCb/C98zZF0+1Ky+qgV2zmZD2mkrTuIQaeRhJpOA1yslhGaK 4z8y0dwGQ5CzKAUM4XHVnmrV1hc2+TO9dLqDSa9bnRC/RdTYnj5M1+VI73X7TrHz5EUTZZ jVjrgOd4l6AkoaGEqGf369G5yuaMhyvKPr49j3H7lYVmDs2RMyynLoI+oY/ZgsMu1Vd9+4 5GedbwG9KXFHZ/cYMv5EtbaWLqrj01mYcyI5FJNd00FgYqJ9IsSHG5E1iNSq3A== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N On 10/23/22 11:18 AM, Mark Johnston wrote: > I'm going through compiler warnings in the bhyve code with the aim of > bumping WARNS, since the current setting hides bugs. There's one in the > config code that looks a bit tricky to resolve and I was hoping for some > guidance on the right way to do that. > > The basic problem is _lookup_config_node() may return an nvlist via > nvlist_get_nvlist(), but nvlist_get_nvlist() returns a const nvlist_t > *, so _lookup_config_node() is discarding the const qualifier. And > indeed, some callers will modify the returned node. The use of > nvlist_move_nvlist() in _lookup_config_node() has a similar problem: > nvlist_move_* "consumes" the passed value and is not supposed to be > mutated after. > > I'm pretty sure this is actually a non-issue with our nvlist > implementation; when adding an nvlist value to an nvlist, it doesn't > store anything except for the pointer itself, so you can mutate it > safely. Note, this is not true for all value types, specifically > arrays. However, there are multiple nvlist implementations out there, > and ours might change such that bhyve's config code becomes incorrect. > > The bug seems annoying to fix because consumers can do this: > > nvlist_t *nvl = find_config_node(path); > set_config_value_node(nvl, "foo", "bar"); > > So, if we naively changed find_config_node() to return a copy of the > nvlist at "path", set_config_value_node() would have to somehow figure > out where in the config tree to insert the updated nvlist, but it > doesn't have the path anymore. > > To fix it properly could change find_config_node() to return some opaque > type which contains the source path of the node so that we can DTRT when > mutating the node. IMO it's better if the config node type is opaque to > consumers. Or, make each nvlist node store its source path by storing > it in the nvlist itself. e.g., the nvlist node describing the device > model at "pci.0.1.2" would have a "__path" key with value "pci.0.1.2", > so that set_config_value_node() can figure out where in the tree to > replace an existing copy of "pci.0.1.2". Or is there a simpler way to > fix this that I missed? > > Any thoughts? I'm happy to implement the suggestions above but didn't > want to do that work without some buy-in. The config stuff uses nvlist on the premise that it is a useful way to store the data. However, it is true that nvlists seem to not be as useful for storing mutable data. Rather, they are useful as a way to pass structured data across domain boundaries (e.g. serialized over a socket, or between kernel and userland). If nvlist doesn't work well I'd rather just change the data structures we use in bhyve rather than add hidden nodes with path names, etc. To that end, if nvlist is too limiting then we can do what my first intution was which was to turn bhyve into a C++ program and use something like std::map<> to represent the tree instead. -- John Baldwin