Re: [PATCH v2 00/16]: hw/i386/vmport: Bug fixes and improvements

[Liran Alon]

On 10/03/2020 22:56, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 08:09:09PM +0200, Liran Alon wrote:
> > On 10/03/2020 19:44, Michael S. Tsirkin wrote:
> > > On Tue, Mar 10, 2020 at 06:53:16PM +0200, Liran Alon wrote:
> > > > Hi,
> > > >
> > > > This series aims to fix several bugs in VMPort and improve it by supporting
> > > > more VMPort commands and make command results more configurable to
> > > > user via QEMU command-line.
> > > >
> > > > This functionality was proven to be useful to run various VMware VMs
> > > > when attempting to run them as-is on top of QEMU/KVM.
> > > >
> > > > For more details, see commit messages.
> > > Well two versions in one day and some review comments weren't addressed.
> > There is a single review comment that wasn't addressed which is replacing an
> > enum with a comment. And I explicitly mentioned that it's because I want
> > additional opinion on this.
> > I don't see why such a small thing should block review for 15 patches...
> > All the rest of the comments (Which were great) have been addressed. Unless
> > I have mistakenly missed something, which please point it out if I did.
> OK I just took a quick peek, two things quickly jumped out at me.
Thanks for having a look.
> version property really should be a boolean and have some documentation
> saying what functionality enables.
I thought that having a version number approach is more generic and easy 
to maintain going forward.
If I understand correctly, this is also the approach taken by qxl & qxl-vga.

The more elaborate alternative could have been introducing compat_flags 
(As PVSCSI does) but it seems like it will pollute the property space 
with a lot of useless VMPort properties.
(E.g. x-read-eax-bug, x-no-report-unsupported-cmd, x-no-report-vmx-type 
and etc.).

What is the advantage of having a boolean such as "x-vmport-v2" instead 
of having a single "version" property?
Will it suffice if I would just add documentation above "version" 
property on what is was the functionality in "version==1"?
(Though, it's just easy to scan the vmport.c code for if's involving 
">version"... "version" is more of an internal field for machine-type 
compatibility and not really meant to be used by user)

Which approach do you prefer?

> userspace properties should use the non-abbreviated
> vm-executable since vmx is easy to confuse with vm extensions.
I really wish you would reconsider this. VMX is a really common term in 
VMware terminology.
It is found in binary names, ".vmx" file, ".vmx" file properties, VMware 
Tools prints, open-vm-tools source code and etc.

In contrast, even though I have dealt for many years with VMware 
technologies, I have never known that VMX==vm-executable.
I still think it will introduce much confusion. On the other hard, I 
don't see much confusing with this use of VMX with Intel VT-x
because it is only used inside vmport.c and in vmport properties names. 
And the properties names match the names of the guest
code that interface with vmport in open-vm-tools source code.

If you still have a strong opinion on this, I will change it as you say 
in v3... But please consider above arguments.
> That's just a quick look.
>
>
> > > Some people do this, try to wear the maintainers out by sheer volume.
> > > It works sometimes but it's not a nice tactic. I personally think it's
> > > worth taking the time to think harder about ways to address all
> > > comments, not try to dismiss them.
> > That's not what I tried to do. I carefully fixed all comments I saw in the
> > review discussion and run tests.
> > The only thing which wasn't addressed is removing an enum and replacing it
> > with a comment.
> > The hint that I try to manipulate maintainers is disrespectful. I assume
> > that this isn't your intention, as we all just want to collaborate together
> > here. No need to make this a personal discussion.
> >
> > If you think that replacing the enum with a comment is a blocker for v2
> > patch-series, I will go ahead and submit v3 with that change.
> Yes IMHO it needs to be fixed but please go over the comments and try to
> address them all as best you can, instead of looking for an explanation
> why the comments were irrelevant and can be dismissed.

I'm not trying to finding explanation on why the comments are irrelevant 
and can be dismissed... It's not my first time contributing code to 
QEMU/KVM...

> Sure someone
> might propose you introduce a bug, and that can't just be addressed, but
> that's not the case here.  Also please do not send multiple revisions of
> a large patchset in a day.  People need time for review.
OK. I will make note of that for next time.
I would have thought maintainers prefer to always have ability to pick 
up the latest version that is ready to avoid reviewing old code that was 
already discussed. Assuming all previous comments were addressed.

Thanks,
-Liran