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

[Luiz Capitulino]

On Wed, 10 Nov 2010 14:33:47 +0100
Markus Armbruster <address@hidden> wrote:

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

Yes, it's doable. But this kind of change will make this series intrusive,
for example, is it really impossible to create !chr->filename via the
normal means (eg. from the user)? What if we break something else with this
change?

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

I don't think so, unless we have a real need for it (and this can be done
later anyway).

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