On Wed, Jan 05, 2011 at 02:55:55PM -0700, Eric Blake wrote:
On 01/05/2011 02:09 PM, Alon Levy wrote:
So, I'm thinking that this XML representation matches the spicevmc chardev:
<devices> <channel type='spicevmc'/> <source port='5903' tlsPort='5904' autoport='no' listen='127.0.0.1'/>
I got you until now - but what's with the port/tlsPort - all of that stuff belongs to the spice flag, and I'm pretty sure is already taken care of by some other tag (I guess <spice> probably?).
Hmm, right now, the only use of spice in XML is under <graphics type='spice'>, and it is the <graphics> element that contains port, tlsPort, autoport, and listen arguments. So we may need to revisit that, and have some way to use a single location for spice parameters shared among all spice-related devices, and a way for both <graphics type='spice'> and <smartcard type='passthrough'> to reference that rather than repeating it. Meaning that spicevmc support just got more difficult, if it involves tweaking <graphics> rather than just adding a new <channel type='spicevmc'> element.
Are you saying that for correctness you want to tie the two elements together? I mean, that's the only possible reason I see. If you want to tie them together to prevent an instance of spicevmc without an instance of graphics of type spice, I understand. I guess (after seeing the patch you sent) there is a verifier to the xml inputs to libvirt that would be able to deduce invalidity in that case? Of course the alternative is to have logic elsewhere, but I see the point in trying to verify the xml input only at one place.
This chardev is totally separate (sure, you need to be using spice for it to make sense, but there is not overlap in parameters, for instance you don't give a port nor a tlsPort to the spicevmc chardev).
Okay, looking more at existing devices, I agree that <channel type='spicevmc'> should only need additional attributes/sub-elements corresponding to parameters appearing directly in the -chardev/-device argument pair handed to qemu. So I'm not sure if <source> makes sense any more, or if <source> would be what ends up pointing to common details about the spice server as shared with the <graphics> element.
<target type='smartcard'/>
This looks right.
</channel> </devices>
In looking more at libvirt XML, I don't see any fields that match id assignments; rather, libvirt auto-assigns unique ids in the form %s%d, category, count, where category matches <channel> and count matches how many <channel> devices are present. That is, the above xml would map to:
qemu -chardev spicevmc,id=smartcard,name=channel1
I hope you meant id=channel1,name=smartcard ? the id needs to be the same as the ccid-passthru uses.
Arrgh - yes, more fallout from me swapping id= and name=, and copying and pasting from the wrong email. You already made it clear that id= is the unique name referenced in some other -device chardev=id location, and that for spicevmc, name= is one of two fixed values (vdagent or smartcard).
But I guess we determined that it's easiest to let the <smartcard mode="passthru"/> already add the spicevmc chardev itself? the usage of "-chardev spicevmc,id=xyz,type=smartcard" is only for a "-device ccid-card-passthru,chardev=xyz", so one won't appear without the other.
Looking even more at existing examples (can you tell that I'm trying to learn more about existing <devices> at the same time as implementing a new one?), I see this good example from qemuxml2argvdata/qemuxml2argv-serial-tcp-chardev:
<serial type='tcp'> <source mode='connect' host='127.0.0.1' service='9999'/> <protocol type='raw'/> <target port='0'/> </serial>
maps to: -chardev socket,id=serial0,host=127.0.0.1,port=9999 -device isa-serial,chardev=serial0
It looks like -chardev was added in qemu 0.12, and that earlier versions used just -device without having to map things back to a chardev; but since smartcard is post-0.12, I only need to worry about the -chardev side of things.
Hmm; getting spicevmc to work seems independent enough of getting <smartcard> implemented by using existing <channel type='tcp'/>, so I'll try to split my libvirt XML improvements into two batches. Should I focus on <channel type='spicevmc'> or on <smartcard> first?
I guess start with the smartcard first? Implement it without dealing with the spicevmc side - i.e. don't implement the passthru type first, or propose it but don't implement it. Then do the spicevmc part.
Yep, that's pretty much what I'm settling on right now - figure out how to map <smartcard> to existing 'tcp' mapping first, then figure out about adding spicevmc support including expanding <smartcard> to handle spicevmc.
I'm not particular on the order - both are required for RHEL 6.1 anyhow, and each is testable without the other (spicevmc with vdagent usage outlined above, and smartcard without spice by using it locally through libvirt).
So, back to thinking about the new <smartcard> element. The <smartcard mode='host'> and <smartcard mode='host-certificates'> modes are simple, since -device ccid-card-emulated has no associated -chardev argument. But if we go with the ideas that <smartcard mode='passthrough'> implies -device ccid-card-passthru and an associated -chardev, and that the only -chardev's worth pairing with a <smartcard> are tcp and spicevmc, then it's simpler to just add a type attribute, and make <smartcard mode='passthrough' type='tcp'> look a lot like <serial type='tcp'> (no <serial> sub-element, but instead have a <source> sub-element directly resembling the <source> sub-element of serial). Meanwhile, <serial> allows a <target> sub-element (which virtual serial port the guest will access as the chardev), whereas <smartcard> would not (the guest sees the smartcard via the emulated card from the usb-ccid bus, and not as a raw serial port). Thus,
<devices> <smartcard mode='passthrough' type='tcp'> <source host='0.0.0.0' port='2001'/> <protocol type='raw'/> </smartcard> </devices>
maps to: -usb -device usb-ccid -chardev socket,server,host=0.0.0.0,port=2001,id=smartcard1,nowait -device ccid-card-passthru,chardev=smartcard1
and I think it leaves the door open for:
<devices> <channel type='spicevmc'> <target type='vdagent'/> </channel> <smartcard mode='passthrough' type='spicevmc'> <target type='smartcard'/> </smartcard> </devices>
to map to: -chardev spicevmc,id=channel1,name=vdagent -device vdagent,... -usb -device usb-ccid -chardev spicevmc,id=smartcard1,name=smartcard -device ccid-card-passthru,chardev=smartcard1
Hopefully this will make more sense once I actually finish coding up the RelaxNG schema validation, and propose a first round patch that documents the proposed XML with examples.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org