| Message ID | 1424122283-12521-12-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 16 Feb 2015 22:36:26 +0100 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Drop duplicate code. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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
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" <mst@redhat.com> wrote: > > > Drop duplicate code. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > 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.
On Wed, 18 Feb 2015 15:55:54 +0100 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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" <mst@redhat.com> wrote: ... > > > 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. But as far as I can see, you also did not do this change in your other patches of this series, so this seems a little bit inconsequent (e.g. virtio-blk.c depends on VIRTIO_ID_BLOCK, but you did not include the virtio_ids.h header there). Thomas
On Wed, Feb 18, 2015 at 04:36:39PM +0100, Thomas Huth wrote: > On Wed, 18 Feb 2015 15:55:54 +0100 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > 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" <mst@redhat.com> wrote: > ... > > > > 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. > > But as far as I can see, you also did not do this change in your other > patches of this series, so this seems a little bit inconsequent (e.g. > virtio-blk.c depends on VIRTIO_ID_BLOCK, but you did not include the > virtio_ids.h header there). > > Thomas OK - I'll do this as a patch on top? Don't want to repost the whole series.
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" #include "qemu/iov.h" #include "monitor/monitor.h" #include "qemu/queue.h"
Drop duplicate code. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/hw/virtio/virtio-serial.h | 40 +-------------------------------------- hw/char/virtio-serial-bus.c | 1 + 2 files changed, 2 insertions(+), 39 deletions(-)