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

[Michael S. Tsirkin]

On Tue, Mar 10, 2020 at 02:29:42PM -0700, Liran Alon wrote:
> 
> 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?

It's not clear what should happen going forward. Let's say version is
incremented again. This then becomes challenging for downstreams to
backport.

> 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?

I just dislike versions, they are hard to maintain.

Individual ones is cleanest imho. Self-documenting.
But if not, I'd do something like "x-vmport-fixes" and
set bits there for each bugfix.


> > 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.

Well that at least is easy to google.

        .vmx

        <vmname>.vmx

        This is the primary configuration file, which stores settings
        chosen in the New Virtual Machine Wizard or virtual machine settings
        editor. If you created the virtual machine under an earlier version of
        VMware Workstation on a Linux host, this file may have a .cfg extension

so .vmx as used here has nothing to do with VM Executable version or
type. Looks like it's just a source of confusion on the vmware
side too :)



> 
> In contrast, even though I have dealt for many years with VMware
> technologies, I have never known that VMX==vm-executable.

Well you said that's what it stands for. I have no idea.  From what you
say now maybe vmx basically is being used as a prefix for all things
vmware. In that case vmport-version and vmport-type or even
vmware-version and vmware-type will do just as well.


> 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.

I'm just saying don't use vmx. It's too late to try to give
it a different meaning. Figure out what it's supposed to
stand for and write it out in full.

> > 
> > 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
>