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

[Michael S. Tsirkin]

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.

version property really should be a boolean and have some documentation
saying what functionality enables.

userspace properties should use the non-abbreviated
vm-executable since vmx is easy to confuse with vm extensions.

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

> Is there any other comment you made on v1 patch-series you think I missed?
> 
> Thanks,
> -Liran
> 
> > 
> > Thanks!
> > 
> > > Regards,
> > > -Liran
> > > 
> > > v1->v2:
> > > * Fix coding convention [Patchew Bot & MST].
> > > * Create new header file for vmport.h [MST].
> > > * Move KVM_APIC_BUS_FREQUENCY from linux-headers/asm-x86/kvm.h
> > >    auto-generated header [MST]
> > > * Elaborate more that vmx-version refers to the VMware userspace
> > >    VMM in commit message. [MST]
> > > * Use le32_to_cpu() on BIOS_UUID vmport command. [MST]
> > > * Introduce VMPort compatability version property to maintain migration
> > >    compatibility. [MST]