On 2/17/22 10:45, Peter Krempa wrote: > On Thu, Feb 17, 2022 at 10:39:59 +0100, Michal Prívozník wrote: >> On 2/16/22 16:41, Peter Krempa wrote: >>> The fix was on RPC level so everything should advertise it. >>> >>> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx> >>> --- >>> src/driver.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/driver.c b/src/driver.c >>> index d070861cfd..54b4ad5b43 100644 >>> --- a/src/driver.c >>> +++ b/src/driver.c >>> @@ -357,10 +357,12 @@ virDriverFeatureIsGlobal(virDrvFeature feat, >>> * At this point everything supports them and thus also drivers need to >>> * always advertise this feature */ >>> case VIR_DRV_FEATURE_TYPED_PARAM_STRING: >>> + /* Feature flag exposes that accidental switching of order of arguments >>> + * in RPC was fixed. All implementations need to advertise this feature */ >>> + case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: >>> *supported = 1; >>> return true; >>> >>> - case VIR_DRV_FEATURE_NETWORK_UPDATE_HAS_CORRECT_ORDER: >>> case VIR_DRV_FEATURE_FD_PASSING: >>> case VIR_DRV_FEATURE_MIGRATION_V2: >>> case VIR_DRV_FEATURE_MIGRATION_V3: >> >> Just FYI, RPC layer doesn't have to necessary be involved. We have embed >> mode which uses drivers directly. Even though, network driver is >> currently doesn't work in embed mode. And strictly speaking this was not >> tied to RPC, the problem was in public API implementation which called >> the callback with swapped arguments. So any of local drivers (remote >> driver, ESX, test driver, ...) were affected too, even though it's the >> remote driver alone that implemented the callback. > > So, the problematic ordering was in the public API wrapper function? Indeed: git show -W b0f78d626a18bcecae3a4d165540ab88bfbfc9ee -- src/libvirt-network.c You'll see that the public API was given @command and then @section arguments, but the callback was called with @section and @command (reversed). This went unnoticed because both arguments are of the same type (thus RPC serializer didn't report any error) and because the public API is called again on the daemon side (during dispatch) so the order was swapped again and thus network driver saw arguments in correct order. Problem arose only when split daemons were introduced -> suddenly the API was called three times (one time at client, one time at virtqemud and finally in virnetworkd), so there was imbalance of swapping. And yes, if client connected to virtnetworkd directly everything worked, because again - two swapps were done. > >> >> I'm not objecting to this patch, just trying to shed more light into the >> problem. > > I can update the comment. Actually the idea is that the comment captures > the reason for the flag, so it should be acurate. > Yeah, it's just that I'm unable to come with anything better. Michal
