Hi Andrew, On Tue, 22 Oct 2019 at 14:43, Andrew Jones <drjo...@redhat.com> wrote: > > On Mon, Oct 21, 2019 at 04:07:14PM +0100, Beata Michalska wrote: > > Indeed, the patch got bit messed-up. Apologies for that as well. > > I have been testing manually but I did try the test you have provided > > and yes it fails - there is a slight problem with the case when qdict_in > > is empty,but this can be easily solved still keeping the single loop. > > Otherwise I have seen you have posted a new patchest so I guess we are > > dropping the idea of refactoring ? > > Well, without a patch that applies, I couldn't really evaluate your > proposal. And, TBH, I'd rather not hold this series up on a refactoring > that doesn't provide measurable performance improvements, especially when > it's not in a performance critical path. Indeed, I'd like to get this > series merged as soon as possible, which is why I posted v6 with your > visit_free() fix already. > > > > > One more question: in case of querying a property which is not supported > > by given cpu model - we are returning properties that are actually valid > > (the test case for cortex-a15 and aarch64 prop). > > Shouldn't we return an error there? I honestly must admit I do not know > > what is the expected behaviour for the qmp query in such cases. > > We do generate an error for that case: > > (QEMU) query-cpu-model-expansion type=full model={"name":"cortex-a15"} > {"return": {"model": {"name": "cortex-a15", "props": {"pmu": true}}}} > > (QEMU) query-cpu-model-expansion type=full > model={"name":"cortex-a15","props":{"aarch64":false}} > {"error": {"class": "GenericError", "desc": "Property '.aarch64' not found"}} > > > If you have any more comments on the series, please send them right away. > I'd like Peter to be able to merge this soon, and I understand that he's > waiting on your review. >
I think we can proceed with the v6 as it is. Thanks a lot. BR Beata > Thanks, > drew >