Re: [Qemu-devel] [PATCH 1/2] qemu-char: Introduce Buffered driver

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 10 Nov 2010 13:56:39 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Wed, 10 Nov 2010 10:26:15 +0100
>> > Markus Armbruster <address@hidden> wrote:
[...]
>> >> Unlike normal character drivers, this one can't be closed with
>> >> qemu_chr_close(), can it?  What happens if someone calls
>> >> qemu_chr_close() on it?
>> >
>> > I guess it will explode, because this driver is not in the chardevs list
>> > and our CharDriverState instance is allocated on the stack.
>> >
>> > Does a function comment solves the problem or do you have something else
>> > in mind?
>> 
>> A general OO rule: Having different constructors for different sub-types
>> is okay, but once constructed, you should be able to use the objects
>> without knowing of what sub-type they are.  That includes destruction.
>
> We will have to add our MemoryDriver to the chardevs list, this has some
> implications like being visible in qemu_chr_info() and qemu_chr_find(),
> likely to also imply that we should choose a chr->filename.

Not if we formalize the notion of an "internal use only" character
device.  Say, !chr->filename means it's internal, and internal ones
aren't in chardevs.  Make qemu_chr_close()'s QTAILQ_REMOVE() conditional
!chr->filename.

> Another detail is that we'll have to dynamically alocate our CharDriverState
> instance. Not a problem, but adds a few more lines of code and a
> qemu_free(). None of this is needed today.

I doubt the alloc/free matters.

> Really worth it?

Your call.

But if you decide not to, please add a suitable assertion to
qemu_chr_close(), to make it obvious what went wrong when an internal
character device explodes there.

>> Exceptions prove the rule.  Maybe this is one, maybe not.
[...]