Re: [Qemu-devel] [PATCH for-2.1 v2 2/2] char: report frontend open/closed state in 'query-chardev'

[Eric Blake]

On 06/26/2014 07:38 AM, Laszlo Ersek wrote:

>>>  ##
>>> -{ 'type': 'ChardevInfo', 'data': {'label': 'str', 'filename': 'str'} }
>>> +{ 'type': 'ChardevInfo', 'data': {'label': 'str',
>>> +                                  'filename': 'str',
>>> +                                  'frontend-open': 'bool'} }
>>
>> Hmm; I wonder if this should instead be
>> 'frontend-status':'VirtioSerialPortStatus', to reuse the type from patch
>> 1/2.  That way, if we ever add a third state, then both the event and
>> the poll will be reusing the same enum values to report that state.
> 
> I expected this remark :)
> 
> The difference is rooted in the fact that the event approaches the
> virtio-serial port status from the frontend (ie. guest) side, while the
> query approaches the same from the backend (ie. host, chardev) side.
> 
> If I wanted to bring those in future-proof sync, then I would have to
> change the underlying, generic chardev machinery -- namely, the fe_open
> field, and everything that operates on it.
> 
> I actually considered the other direction too: rather than introducing
> status:VirtioSerialPortStatus, just add open:bool (which was your
> original suggestion in your v1 review). I decided against it because the
> current list of enum constants (connected, disconnected) expresses just
> the same, and it'll be a *tiny* bit easier to extend, should that
> necessity arise.
> 
> Sounds acceptible? :) If not, then I'm OK to replace
> status:VirtioSerialPortStatus with open:bool in the first patch.

If we ever add another state in the future, then both places would be
touched at the same time to figure out how to support that new state.
So I agree with your counter-proposal of just simplifying things to use
open:bool in the first patch; it also has the benefit of less code (no
need to add an enum).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature