[Spice-devel] [PATCH spice v2 1/1] rename the virtio port for streaming

[Frediano Ziglio]

> On Mon, 2018-03-26 at 08:31 -0400, Frediano Ziglio wrote:
> > > 
> > > On Fri, 2018-03-23 at 11:09 -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > From: Lukáš Hrázký <lukkash at email.cz>
> > > > > 
> > > > > The name 'com.redhat.stream.0' is too generic and in no way denotes
> > > > > it
> > > > > belongs to SPICE. It is preferred to have the project's domain in the
> > > > > name and Red Hat doesn't own the project. Rename it to
> > > > > org.spice-space.stream.0.
> > > > > 
> > > > > Signed-off-by: Lukáš Hrázký <lukkash at email.cz>
> > > > > ---
> > > > >  server/reds.c                     | 2 +-
> > > > >  server/tests/test-stream-device.c | 2 +-
> > > > >  spice-common                      | 2 +-
> > > > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/server/reds.c b/server/reds.c
> > > > > index 998f2ffa..935448d8 100644
> > > > > --- a/server/reds.c
> > > > > +++ b/server/reds.c
> > > > > @@ -3145,7 +3145,7 @@ static int
> > > > > spice_server_char_device_add_interface(SpiceServer *reds,
> > > > >      else if (strcmp(char_device->subtype, SUBTYPE_PORT) == 0) {
> > > > >          if (strcmp(char_device->portname,
> > > > >          "org.spice-space.webdav.0") ==
> > > > >          0)
> > > > >          {
> > > > >              dev_state = spicevmc_device_connect(reds, char_device,
> > > > >              SPICE_CHANNEL_WEBDAV);
> > > > > -        } else if (strcmp(char_device->portname,
> > > > > "com.redhat.stream.0")
> > > > > ==
> > > > > 0) {
> > > > > +        } else if (strcmp(char_device->portname,
> > > > > "org.spice-space.stream.0")
> > > > > == 0) {
> > > > >              dev_state = RED_CHAR_DEVICE(stream_device_connect(reds,
> > > > >              char_device));
> > > > >          } else {
> > > > >              dev_state = spicevmc_device_connect(reds, char_device,
> > > > >              SPICE_CHANNEL_PORT);
> > > > > diff --git a/server/tests/test-stream-device.c
> > > > > b/server/tests/test-stream-device.c
> > > > > index 3c9209a4..2fdd0a39 100644
> > > > > --- a/server/tests/test-stream-device.c
> > > > > +++ b/server/tests/test-stream-device.c
> > > > > @@ -107,7 +107,7 @@ static SpiceCharDeviceInterface vmc_interface = {
> > > > >  // this specifically creates a stream device
> > > > >  static SpiceCharDeviceInstance vmc_instance = {
> > > > >      .subtype = "port",
> > > > > -    .portname = "com.redhat.stream.0",
> > > > > +    .portname = "org.spice-space.stream.0",
> > > > >  };
> > > > >  
> > > > >  static uint8_t *add_stream_hdr(uint8_t *p, StreamMsgType type,
> > > > >  uint32_t
> > > > >  size)
> > > > > diff --git a/spice-common b/spice-common
> > > > > index 4c2d0e97..45e28448 160000
> > > > > --- a/spice-common
> > > > > +++ b/spice-common
> > > > > @@ -1 +1 @@
> > > > > -Subproject commit 4c2d0e977272c5540634d24f485dd64c424f6748
> > > > > +Subproject commit 45e2844845242b32b2bd8956da0dfffa91c0d856
> > > > 
> > > > I think spice-common part is accidental, besides this
> > > 
> > > Yes, I'll fix it...
> > > 
> > > > Acked-by: Frediano Ziglio <fziglio at redhat.com>
> > > > 
> > > > I would however wait to see what other people think about it.
> > > 
> > > The rest of the series could use an Ack, so that I can merge it
> > > simultaneously. I'll still wait a few more days.
> > > 
> > > > As a follow up maybe a string constant in server/spice-char.h?
> > > 
> > > I don't find it useful to create a constant in another file as long as
> > > it's used in only one place and the other virtio port names here are
> > > also literals...
> > > 
> > > > I noted that there's also a reference to this name in spice-prototol,
> > > > spice/stream-device.h file.
> > > 
> > > I've added a patch to remove it to the series, haven't thought of
> > > looking for it there.
> > > 
> > > Thanks,
> > > Lukas
> > > 
> > 
> > Was thinking about the name, is used on both server and guest.
> > Maybe the constant should be defined in spice-protocol
> > spice/stream-device.h?
> > This would be a single place as you could use that constant in both server
> > and agent.
> > 
> > Something like
> > 
> > /* name of the port */
> > #define STREAM_DEVICE_PORT_NAME "org.spice-space.stream.0"
> > 
> > ??
> > I would than remove the "The name of the port is "... from the
> > stream-device.h header
> > comment.
> 
> I am not sure at all whether the port name should be part of the
> protocol package. If someone has a strong opinion, I'm willing to put
> it there, otherwise I'd keep it as it is, which is also consistent with
> the other port names (which aren't defined in a single common
> location).
> 
> Cheers,
> Lukas
> 

I considered this more of a follow up or improve instead of a requirement,
consider my ack still in place.

Frediano