Re: [Qemu-devel] [PATCH v3 11/17] virtio-serial: switch to standard-headers

[Michael S. Tsirkin]

On Wed, Feb 18, 2015 at 03:34:13PM +0100, Thomas Huth wrote:
> On Mon, 16 Feb 2015 22:36:26 +0100
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > Drop duplicate code.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  include/hw/virtio/virtio-serial.h | 40 
> > +--------------------------------------
> >  hw/char/virtio-serial-bus.c       |  1 +
> >  2 files changed, 2 insertions(+), 39 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio-serial.h 
> > b/include/hw/virtio/virtio-serial.h
> > index 11af978..ccf8459 100644
> > --- a/include/hw/virtio/virtio-serial.h
> > +++ b/include/hw/virtio/virtio-serial.h
> > @@ -15,53 +15,15 @@
> >  #ifndef _QEMU_VIRTIO_SERIAL_H
> >  #define _QEMU_VIRTIO_SERIAL_H
> > 
> > +#include "standard-headers/linux/virtio_console.h"
> >  #include "hw/qdev.h"
> >  #include "hw/virtio/virtio.h"
> > 
> > -/* == Interface shared between the guest kernel and qemu == */
> > -
> > -/* The Virtio ID for virtio console / serial ports */
> > -#define VIRTIO_ID_CONSOLE          3
> > -
> > -/* Features supported */
> > -#define VIRTIO_CONSOLE_F_MULTIPORT 1
> > -
> > -#define VIRTIO_CONSOLE_BAD_ID           (~(uint32_t)0)
> > -
> > -struct virtio_console_config {
> > -    /*
> > -     * These two fields are used by VIRTIO_CONSOLE_F_SIZE which
> > -     * isn't implemented here yet
> > -     */
> > -    uint16_t cols;
> > -    uint16_t rows;
> > -
> > -    uint32_t max_nr_ports;
> > -} QEMU_PACKED;
> > -
> > -struct virtio_console_control {
> > -    uint32_t id;           /* Port number */
> > -    uint16_t event;                /* The kind of control event (see 
> > below) */
> > -    uint16_t value;                /* Extra information for the key */
> > -};
> > -
> >  struct virtio_serial_conf {
> >      /* Max. number of ports we can have for a virtio-serial device */
> >      uint32_t max_virtserial_ports;
> >  };
> > 
> > -/* Some events for the internal messages (control packets) */
> > -#define VIRTIO_CONSOLE_DEVICE_READY        0
> > -#define VIRTIO_CONSOLE_PORT_ADD            1
> > -#define VIRTIO_CONSOLE_PORT_REMOVE 2
> > -#define VIRTIO_CONSOLE_PORT_READY  3
> > -#define VIRTIO_CONSOLE_CONSOLE_PORT        4
> > -#define VIRTIO_CONSOLE_RESIZE              5
> > -#define VIRTIO_CONSOLE_PORT_OPEN   6
> > -#define VIRTIO_CONSOLE_PORT_NAME   7
> > -
> > -/* == In-qemu interface == */
> > -
> >  #define TYPE_VIRTIO_SERIAL_PORT "virtio-serial-port"
> >  #define VIRTIO_SERIAL_PORT(obj) \
> >       OBJECT_CHECK(VirtIOSerialPort, (obj), TYPE_VIRTIO_SERIAL_PORT)
> > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > index 47fbb34..a2bac9b 100644
> > --- a/hw/char/virtio-serial-bus.c
> > +++ b/hw/char/virtio-serial-bus.c
> > @@ -18,6 +18,7 @@
> >   * GNU GPL, version 2 or (at your option) any later version.
> >   */
> > 
> > +#include "standard-headers/linux/virtio_ids.h"
> 
> Could you omit this include? Later in virtio-serial-bus.c, the code
> includes hw/virtio/virtio-serial.h, which in turn includes
> standard-headers/linux/virtio_console.h - and that one already includes
> the ids.h file. So I think it should work without the above change, too.
> 
>  Thomas

Yes but it's generally not a good idea to depend on headers
including each other.

-- 
MST