Re: [spice-server 1/3] test_vdagent: Pass proper types to spice_server_add_interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]