Am 16.06.25 um 11:41 schrieb Daniel P. Berrangé: > On Thu, May 15, 2025 at 01:29:07PM +0200, Fiona Ebner wrote: >> @@ -4327,6 +4360,9 @@ >> # authentication. This maps to Ceph configuration option "key". >> # (Since 3.0) >> # >> +# @key-value-pairs: Key-value pairs for additional Ceph configuraton. >> +# (Since 10.1) >> +# >> # @server: Monitor host address and port. This maps to the "mon_host" >> # Ceph option. >> # >> @@ -4342,6 +4378,7 @@ >> '*user': 'str', >> '*auth-client-required': ['RbdAuthMode'], >> '*key-secret': 'str', >> + '*key-value-pairs' : 'RbdKeyValuePairs', > > I'm not seeing any point in this 'RbdKeyValuePairs' struct. Why isn't > the 'rbd-cache-policy' field just directly part of the BlockdevOptionsRbd > struct like all the other options are ? > > Also, 'rbd-' as a prefix in the field name is redundant when this is > already in an RBD specific struct.
Am 16.06.25 um 11:25 schrieb Ilya Dryomov: > Hi Fiona, > > I'm not following the rationale for introducing RbdKeyValuePairs > struct. If there is a desire to expose rbd_cache_policy option this > way, couldn't it just be added to BlockdevOptionsRbd struct? The > existing auth_client_required option has a very similar pattern. Hi Ilya and Daniel, my intention was to not "pollute" the top-level struct with rather uncommon options, but collect them separately and also make it explicit that those are the key-value pairs passed along to Ceph. > If exposing rbd_cache_policy option, rbd_cache option (enabled/disabled > bool) should likely be exposed as well. It doesn't make much sense to > provide a built-in way to adjust the cache policy without also providing > a built-in way to disable the cache entirely. Then the question of what > would be better from the QAPI perspective arises: a bool option to map > to Ceph as close as possible or perhaps an additional 'disabled' value > in RbdCachePolicy enum? And regardless of that, the need to remember > to update QEMU if a new cache policy is ever added because even though > these are just strings, QEMU is going to be validating them... Good point! If going with the key-value-pairs approach, it would be nicer to go with a direct mapping IMHO. If adding it to the top-level, I'd be in favor of the 'disabled' value. Best Regards, Fiona
