[Spice-devel] [spice-server 1/3] test_vdagent: Pass proper types to spice_server_add_interface
Frediano Ziglio
fziglio at redhat.com
Thu Feb 2 13:26:10 UTC 2017
>
> On Tue, Jan 31, 2017 at 12:15:13PM -0500, Frediano Ziglio wrote:
> > >
> > > On Tue, Jan 31, 2017 at 07:45:44AM -0500, Frediano Ziglio wrote:
> > > > Title should be test-vdagent (with dash).
> > >
> > > Thanks, fixed.
> > >
> > > >
> > > > >
> > > > > This is a revert of 93b4f4050^ and 93b4f4050.
> > > > > For a SpiceCharDeviceInstance, the base interface must be a
> > > > > SpiceCharDeviceInterface instance, not a SpiceBaseInterface instance,
> > > > > or
> > > > > spice-server code will end up reading out of bounds.
> > > > >
> > > > > vmc_state/vmc_write/vmc_read implementations also have to be
> > > > > provided.
> > > > > ---
> > > > > server/tests/test-vdagent.c | 65
> > > > > ++++++++++++++++++++++++++++++++++++++++-----
> > > > > 1 file changed, 59 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/server/tests/test-vdagent.c
> > > > > b/server/tests/test-vdagent.c
> > > > > index 7f905ad..e06229e 100644
> > > > > --- a/server/tests/test-vdagent.c
> > > > > +++ b/server/tests/test-vdagent.c
> > > > > @@ -45,11 +45,64 @@ static void pinger(SPICE_GNUC_UNUSED void
> > > > > *opaque)
> > > > > core->timer_start(ping_timer, ping_ms);
> > > > > }
> > > > >
> > > > > -static SpiceBaseInterface base = {
> > > > > - .type = SPICE_INTERFACE_CHAR_DEVICE,
> > > > > - .description = "test spice virtual channel char device",
> > > > > - .major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
> > > > > - .minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
> > > > > +static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
> > > > > + SPICE_GNUC_UNUSED const uint8_t *buf,
> > > > > + int len)
> > > > > +{
> > > > > + return len;
> > > > > +}
> > > > > +
> > > > > +static int vmc_read(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
> > > > > + uint8_t *buf,
> > > > > + int len)
> > > > > +{
> > > > > + static uint8_t c = 0;
> > > > > + static uint8_t message[2048];
> > > > > + static unsigned pos = 0;
> > > > > + static unsigned message_size;
> > > > > + int ret;
> > > > > +
> > > > > + if (pos == 0) {
> > > > > + VDIChunkHeader *hdr = (VDIChunkHeader *)message;
> > > > > + VDAgentMessage *msg = (VDAgentMessage *)&hdr[1];
> > > > > + uint8_t *p = message;
> > > > > + int size = sizeof(message);
> > > > > + message_size = size;
> > > > > + /* fill in message */
> > > > > + hdr->port = VDP_SERVER_PORT;
> > > > > + hdr->size = message_size - sizeof(VDIChunkHeader);
> > > > > + msg->protocol = VD_AGENT_PROTOCOL;
> > > > > + msg->type = VD_AGENT_END_MESSAGE;
> > > > > + msg->opaque = 0;
> > > > > + msg->size = message_size - sizeof(VDIChunkHeader) -
> > > > > sizeof(VDAgentMessage);
> > > > > + size -= sizeof(VDIChunkHeader) + sizeof(VDAgentMessage);
> > > > > + p += sizeof(VDIChunkHeader) + sizeof(VDAgentMessage);
> > > > > + for (; size; --size, ++p, ++c)
> > > > > + *p = c;
> > > > > + }
> > > > > + ret = MIN(message_size - pos, len);
> > > > > + memcpy(buf, &message[pos], ret);
> > > > > + pos += ret;
> > > > > + if (pos == message_size) {
> > > > > + pos = 0;
> > > > > + }
> > > > > + //printf("vmc_read %d (ret %d)\n", len, ret);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static void vmc_state(SPICE_GNUC_UNUSED SpiceCharDeviceInstance
> > > > > *sin,
> > > > > + SPICE_GNUC_UNUSED int connected)
> > > > > +{
> > > > > +}
> > > > > +
> > > > > +static SpiceCharDeviceInterface vmc_interface = {
> > > > > + .base.type = SPICE_INTERFACE_CHAR_DEVICE,
> > > > > + .base.description = "test spice virtual channel char device",
> > > > > + .base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
> > > > > + .base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
> > > > > + .state = vmc_state,
> > > > > + .write = vmc_write,
> > > > > + .read = vmc_read,
> > > > > };
> > > > >
> > > > > SpiceCharDeviceInstance vmc_instance = {
> > > > > @@ -63,7 +116,7 @@ int main(void)
> > > > > core = basic_event_loop_init();
> > > > > test = test_new(core);
> > > > >
> > > > > - vmc_instance.base.sif = &base;
> > > > > + vmc_instance.base.sif = &vmc_interface.base;
> > > > > spice_server_add_interface(test->server, &vmc_instance.base);
> > > > >
> > > > > ping_timer = core->timer_add(pinger, NULL);
> > > >
> > > > How did you discover this problem?
> > >
> > > > This test were supposed to be working even without vmc_read, why now
> > > > is needed? It's not exactly a stub (like "return -1").
> > > > Maybe NULLs are fine too?
> > >
> > > s/SpiceBaseInstance/SpiceCharDeviceInstance/ was found because the test
> > > was crashing at start. vmc_read implementation is the same as the one
> > > which was present before 93b4f4050. Arguably, it's not really useful as
> > > then the test just loops forever on vmc_read, but I'd say that's a
> > > different issue (and actually I don't really know what the test is
> > > supposed to be testing /o\ )
> > > Without a vmc_read implementation, you get a crash when trying to read
> > > from the device in vdi_port_read_one_msg_from_device(). There used to be
> > > an assert when one of the vfuncs was missing, which alos caused me to
> > > add this.
> > >
> > > Christophe
> > >
> >
> > I don't know what this test is supposed to do but change a crash
> > into an infinite loop.
> >
> > Original commit message:
> >
> > add server/tests/test_vdagent
> >
> > File comment:
> >
> > Test vdagent guest to server messages
> >
> > suggestions of what should be the behaviour?
>
> Might make more sense as:
>
>
> diff --git a/server/tests/test-vdagent.c b/server/tests/test-vdagent.c
> index e06229e..bbe5451 100644
> --- a/server/tests/test-vdagent.c
> +++ b/server/tests/test-vdagent.c
> @@ -37,14 +37,6 @@ int ping_ms = 100;
> #define MIN(a, b) ((a) > (b) ? (b) : (a))
> #endif
>
> -static void pinger(SPICE_GNUC_UNUSED void *opaque)
> -{
> - // show_channels is not thread safe - fails if disconnections /
> connections occur
> - //show_channels(server);
> -
> - core->timer_start(ping_timer, ping_ms);
> -}
> -
> static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
> SPICE_GNUC_UNUSED const uint8_t *buf,
> int len)
> @@ -62,6 +54,10 @@ static int vmc_read(SPICE_GNUC_UNUSED
> SpiceCharDeviceInstance *sin,
> static unsigned message_size;
> int ret;
>
> + g_warn_if_fail(pos <= sizeof(message));
> + if (pos == sizeof(message)) {
> + return 0;
> + }
> if (pos == 0) {
> VDIChunkHeader *hdr = (VDIChunkHeader *)message;
> VDAgentMessage *msg = (VDAgentMessage *)&hdr[1];
> @@ -81,11 +77,9 @@ static int vmc_read(SPICE_GNUC_UNUSED
> SpiceCharDeviceInstance *sin,
> *p = c;
> }
> ret = MIN(message_size - pos, len);
> + g_warning("sending %i bytes", ret);
> memcpy(buf, &message[pos], ret);
> pos += ret;
> - if (pos == message_size) {
> - pos = 0;
> - }
> //printf("vmc_read %d (ret %d)\n", len, ret);
> return ret;
> }
> @@ -119,10 +113,5 @@ int main(void)
> vmc_instance.base.sif = &vmc_interface.base;
> spice_server_add_interface(test->server, &vmc_instance.base);
>
> - ping_timer = core->timer_add(pinger, NULL);
> - core->timer_start(ping_timer, ping_ms);
> -
> - basic_event_loop_mainloop();
> -
> return 0;
> }
>
>
> Christophe
>
I manage to get at the commit when this test was introduced and compile
everything (less easy that it seems...).
The test it's stuck in an infinite loop in spice_server_add_interface.
(gdb) bt
#0 0x0000000000403cf4 in vmc_read (sin=0x607310 <vmc_instance>, buf=0x60d958 "\002", len=8) at test_vdagent.c:60
#1 0x00007ffff7afc474 in vdi_port_read_one_msg_from_device (sin=0x607310 <vmc_instance>, opaque=0x0) at reds.c:903
#2 0x00007ffff7aba387 in spice_char_device_read_from_device (dev=0x618e50) at char_device.c:296
#3 0x00007ffff7abbaef in spice_char_device_start (dev=0x618e50) at char_device.c:764
#4 0x00007ffff7b04997 in spice_server_char_device_add_interface (s=0x60d8d0, sin=0x607310 <vmc_instance>) at reds.c:3701
#5 0x00007ffff7b0503d in spice_server_add_interface (s=0x60d8d0, sin=0x607310 <vmc_instance>) at reds.c:3813
#6 0x0000000000403db8 in main () at test_vdagent.c:99
No clue what was the intention.
Surely your change at least do something... but what are they testing?
Looks like an initial attempt to check something for the agent.
At the current state I would just remove the test.
Frediano
More information about the Spice-devel
mailing list