[pve-devel] [PATCH qemu-server v2 0/3] Fix #2041 and #413

[Thomas Lamprecht]

Hi,

On 1/10/19 9:51 AM, Andreas Steinel wrote:
> Hi all,
> 
> On Wed, Jan 9, 2019 at 10:34 AM Dominik Csapak <d.csapak at proxmox.com> wrote:
> 
>> thanks for the patches,
>> generally look ok, but a few high level things:
>>
>> it would be nicer to not introduce breaking changes which gets fixed
>> with the next patch, better would be:
>>
>> 1/3 add option
>> 2/3 add x when option is set
>> 3/3 add y when option is set
>>
>> this way, there is no breaking change introduced
>>
> 
> Yeah, they are the changes I made in my feature branch. I'm unfamiliar with
> git-email,
> so this is maybe due to some strange side effects. I'm used to do pull
> request, so the
> owner can just merge the branch as a single commit, so that the patch
> supplier does
> not need to take take of merging it beforehand.

git rebase can really help with this, you can reorder, squash/fixup or edit
commit ranges relatively easily there. Also, for pull requests a correct order
and splitted up by logical-belonging-together is quite nice, projects which
squash a whole request into a single commit do not sound enjoyable to review,
to be honest :)

> Next time I tried to create a new branch and merge all my changes into that
> one and
> do a git-email stuff afterwards.
> 
> 
>> also i am not sure if we want to link the audio device so closely with
>> spice? @all is there a good use case for having and audio devices
>> without spice? (vnc has no audio, rdp does not need a device, with
>> gpu passthrough you most likely have an hdmi audio or usb audio device,
>> so that only leaves spice?)
>>
> 
> I concur.
> 
> then as alexandre mentioned, this is only necessary with non q35
>> machines, as we have there an intel-hda audio device anyway
>> (we could do this as a follow up though)
>>
>> also i think the logic for the winversion is in reverse,
>> as i think linux has support for intel hda?
>>
>> so we probably want:  winver < 6 -> AC97 else intel hda ?
>> or is there any special reason why non windows machines get ac97?
>>
> 
> Yes, that makes sense. I try to implement the q35 thing.
> 

Did you had time to look into this, we'd still like to see such a feature
you proposed.