On (Mon) Nov 30 2009 [12:27:21], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:31 pm Amit Shah wrote:
> > From: Rusty Russell <[email protected]>
> >
> > Now we can use an allocation function to remove our global console variable.
> >
> > Signed-off-by: Rusty Russell <[email protected]>
> > Signed-off-by: Amit Shah <[email protected]>
> > ---
> > drivers/char/virtio_console.c | 70
> > +++++++++++++++++++++++++++-------------
> > 1 files changed, 47 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index c133bf6..98a5249 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -32,6 +32,18 @@
> > * across multiple devices and multiple ports per device.
> > */
> > struct ports_driver_data {
> > + /*
> > + * This is used to keep track of the number of hvc consoles
> > + * spawned by this driver. This number is given as the first
> > + * argument to hvc_alloc(). To correctly map an initial
> > + * console spawned via hvc_instantiate to the console being
> > + * hooked up via hvc_alloc, we need to pass the same vtermno.
> > + *
> > + * We also just assume the first console being initialised was
> > + * the first one that got used as the initial console.
> > + */
> > + unsigned int next_vtermno;
>
> Let's just make this a global?
There will be a few more globals introduced in the next few patches, so
I just put all the globals in a new struct to keep them in one place.
> > +static struct port *__devinit add_port(u32 vtermno)
> > +{
> > + struct port *port;
> > +
> > + port = kzalloc(sizeof *port, GFP_KERNEL);
> > + if (!port)
> > + return NULL;
> > +
> > + port->used_len = port->offset = 0;
> > + port->inbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > + if (!port->inbuf) {
> > + kfree(port);
> > + return NULL;
> > + }
> > + port->hvc = NULL;
> > + port->vtermno = vtermno;
> > + return port;
> > +}
>
> Rename this to add_console_port(),
I do that later (in patch 15), splitting this off into
init_console_port().
> and split off the core part which
> does ->port setup into "int setup_port(struct port *)" for later re-use?
yeah; this function then becomes the core port init routine in patch 15.
> > +static void free_port(struct port *port)
> > +{
> > + kfree(port->inbuf);
> > + kfree(port);
> > +}
>
> Similarly, free_console_port/free_port?
This comes with port hot unplug support later in the series. This part
is again what I left intact from your series.
> > + err = -ENOMEM;
> > + port = add_port(pdrvdata.next_vtermno);
> > + if (!port)
> > + goto fail;
>
> I realize other coders do this pre-init, and it saves a line, but it also
> means that you don't get a warning about err being uninitialized if it doesn't
> get set correctly later on.
>
> So I prefer:
> port = add...
> if (!port) {
> err = -ENOMEM;
> goto fail;
> }
Sure; I can do that. I just made sure all the style was consistent.
Amit
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization
