Re: [Spice-devel] [PATCH win-vdagent] Don't exit when receiving unknown messages
> On Thu, 2019-02-28 at 03:31 -0500, Frediano Ziglio wrote:
> > >
> > > In 8251fa25, a check on the minimum size of a message was
> > > introduced.
> > > For unsupported messages, the vdagent simply exited. This makes it
> > > difficult to extend the vdagent protocol without breaking old
> >
> > The protocol is using capabilities, there should not be
> > unsupported messages.
> >
> > > installations. Instead, just print a warning indicating that an
> > > unsupported message was received and ignore it.
> > >
> >
> > If you want to print a warning it means that it's not correct,
> > this is another indication that it's wrong.
> >
> > > Signed-off-by: Jonathon Jongsma
> > > ---
> > > vdagent/vdagent.cpp | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> > > index 89019bb..177e663 100644
> > > --- a/vdagent/vdagent.cpp
> > > +++ b/vdagent/vdagent.cpp
> > > @@ -1288,8 +1288,7 @@ void
> > > VDAgent::dispatch_message(VDAgentMessage* msg,
> > > uint32_t port)
> > > break;
> > > }
> > > if (min_size < 0) {
> > > -vd_printf("Unsupported message type %u size %u", msg-
> > > >type,
> > > msg->size);
> > > -_running = false;
> > > +vd_printf("Unsupported message type %u size %u, ignoring",
> > > msg->type, msg->size);
> > > return;
> > > }
> > > if (msg->size < (unsigned) min_size) {
> >
> > RHEL 8 has 8251fa25 patch, should the users be forced to update
> > the guests?
> > If we want to change the protocol to allow unsupported message we
> > should first change the protocol specification, this is not a
> > fix, it's a workaround for a wrong/incomplete implementation on
> > the server side.
> > I prefer a proper fix than a workaround and this is not
> > fixing already installed guests.
> >
> > Frediano
>
>
> Well, I didn't necessarily mean that this is a full solution, but I
> think it is still an improvement. In fact, this was the behavior that
> vdagent-win had before commit 8251fa25. For example, later on in this
> function when we're handling the messages, there is the following code:
>
> default:
> vd_printf("Unsupported message type %u size %u", msg->type,
> msg->size);
> }
>
> The agent does not exit due to an unsupported message. It just prints a
> warning.
>
> In addition, this is the same behavior as the linux vdagent. In linux
> src/vdagentd/vdagentd.c function virtio_port_read_complete():
>
> default:
> g_warn_if_reached();
> }
>
> return 0;
>
> So: Linux vdagent prints a warning, but the vdagent does not exit.
>
> You say above that my change requires a change to the protocol, but in
> fact this is how both vdagents worked for years until commit 8251fa25
> made the windows agent exit when it encounters unsupported messages. So
> I would argue that patch 8251fa25 actually introduced a regression in
> RHEL 8.0.
>
> Jonathon
>
>
Hi,
I'm not that strong on code but I'm pretty strong on the rationale.
Coherence between Linux and Windows agent is fine, a more graceful
handling is fine.
The wrong part is: "This makes it difficult to extend the vdagent
protocol without breaking old installations.", this seems to indicate
that to extend the protocol is good to have unsupported messages.
IMO it would have been better if also the Linux agent would exit,
it would help us detecting the issue easily and avoid the regression
on the server.
What would happen for new messages that needs a reply? It would create
two set of messages, some that should be coded paying attention to
capabilities and some not. And the warnings would be useless, hard
to understand for a customer if the logs are a problem or not.
About patch 8251fa25 introducing a regression then if the current
behaviour for a message handling is to crash the server than
fixing the crash is a regression too. The behaviour on undefined
cases should not invalidate the protocol, you are confusing
protocol definition with protocol implementation.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server 2/2] Only send device display info to supported agents
>
> Only send the graphics device display info to agents that advertise the
> VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability
>
> Signed-off-by: Jonathon Jongsma
> ---
> server/reds-private.h | 1 +
> server/reds.c | 9 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/server/reds-private.h b/server/reds-private.h
> index 9dbc7fa94..b2b99b50e 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -134,6 +134,7 @@ struct RedsState {
> GList *qxl_instances;
> MainDispatcher *main_dispatcher;
> RedRecord *record;
> +gboolean agent_graphics_device_info;
Style: bool
I would prefer a agent_supports_graphics_device_info (or a comment).
But more than this I think it's on the wrong structure, it should be
in RedCharDeviceVDIPortPrivate where the state of the agent is stored.
> };
>
> #define FOREACH_QXL_INSTANCE(_reds, _qxl) \
> diff --git a/server/reds.c b/server/reds.c
> index 63bfadb22..4228ab6ad 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -813,6 +813,10 @@ static void reds_adjust_agent_capabilities(RedsState
> *reds, VDAgentMessage *mess
> if (!reds->config->agent_file_xfer) {
> VD_AGENT_SET_CAPABILITY(capabilities->caps,
> VD_AGENT_CAP_FILE_XFER_DISABLED);
> }
> +
> +size_t caps_size = VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message->size);
> +reds->agent_graphics_device_info =
> VD_AGENT_HAS_CAPABILITY(capabilities->caps, caps_size,
> +
> VD_AGENT_CAP_GRAPHICS_DEVICE_INFO);
The "adjust_agent_capabilities" is no more appropriate now, I would
change in "filter_agent_capabilities". Even better to have another
function to handle capabilities (see below).
OT: Note that VD_AGENT_SET_CAPABILITY above could overflow the message,
in this case not much of an issue, the buffer is statically allocated
in a structure and the change will stay in that buffer.
> }
>
> /* reads from the device till completes reading a message that is addressed
> to the client,
> @@ -965,6 +969,11 @@ void reds_send_device_display_info(RedsState *reds)
> if (!reds->agent_dev->priv->agent_attached) {
> return;
> }
> +if (!reds->agent_graphics_device_info) {
> +g_debug("Not sending device display info to the agent because the
> agent does not support it");
> +return;
> +}
> +
> g_debug("Sending device display info to the agent:");
>
> SpiceMarshaller *m = spice_marshaller_new();
I think this change will cause the message to not be sent in case the agent
connect
as the variable will be false at the beginning, when the capabilities are
received
should be sent.
The variable should be reset also that the agent disconnect (reds_reset_vdp)
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v7 3/4] drm/qxl: remove conflicting framebuffers earlier
Add error checking while being at it. Signed-off-by: Gerd Hoffmann Reviewed-by: Daniel Vetter --- drivers/gpu/drm/qxl/qxl_drv.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index bb81e310eb6d..578d867a81d5 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -79,6 +79,10 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto free_dev; + ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl"); + if (ret) + goto disable_pci; + ret = qxl_device_init(qdev, &qxl_driver, pdev); if (ret) goto disable_pci; @@ -94,7 +98,6 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto modeset_cleanup; - drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl"); drm_fbdev_generic_setup(&qdev->ddev, 32); return 0; -- 2.9.3 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [linux/vd-agent v2 2/3] gitlab-ci: remove copr dependency
> From: Victor Toso > > Instead of relying in copr builds, we should rely on relative branch > in gitlab for spice dependencies such as spice-protocol. > > This patch moves the dependencies to a variable and builds > spice-protocol instead of using spice-protocol from nightly spice > copr. > > Signed-off-by: Victor Toso Acked-by: Frediano Ziglio > --- > .gitlab-ci.yml | 16 ++-- > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index ad4fb80..9b894eb 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -1,12 +1,16 @@ > image: fedora:latest > > +variables: > + DEPS_COMMON: git libtool make python3 python3-six redhat-rpm-config > + bzip2 python3-pyparsing meson ninja-build gtk-doc glib2-devel > + gettext gettext-devel libpciaccess-devel alsa-lib-devel > + libXfixes-devel libX11-devel libXrandr-devel libXinerama-devel > + gtk3-devel dbus-devel systemd-devel > + > before_script: > - - > > -dnf install 'dnf-command(copr)' git libtool make redhat-rpm-config bzip2 > -python3 python3-six python3-pyparsing > --y > - - dnf copr enable @spice/nightly -y > - - dnf builddep spice-vdagent -y > + - dnf install -y $DEPS_COMMON > + - git clone https://gitlab.freedesktop.org/spice/spice-protocol.git > + - (cd spice-protocol && ./autogen.sh --prefix=/usr && make install) > > make-check-distcheck: >script: ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [linux/vd-agent v2 3/3] gitlab-ci: run singular job for build options
> From: Victor Toso > > There are two main bottlenecks in CI: > 1. Network might be slow for bootstrapping; > 2. Big job queues to get a runners to run our jobs. > > This patch reduces the three jobs to a single one in order to prevent > long delays when CI resources are scarce which is a common scenario. > > No major losses in log should happen. We might add specific .log > files as artifacts if we need them later on. > > For completeness, use git clean -xfd for a clean git tree instead of > make distclean. > > Signed-off-by: Victor Toso Acked-by: Frediano Ziglio > --- > .gitlab-ci.yml | 14 +- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 9b894eb..1d31b0d 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -12,32 +12,28 @@ before_script: >- git clone https://gitlab.freedesktop.org/spice/spice-protocol.git >- (cd spice-protocol && ./autogen.sh --prefix=/usr && make install) > > -make-check-distcheck: > +fedora-autotools: >script: >- ./autogen.sh >- make >- make check >- make distcheck > - - make distclean > + - git clean -xfd > > -configure-optional-packages: > - script: >- ./autogen.sh --with-session-info=systemd >--with-init-script=systemd+redhat >- make >- make install >- make uninstall > - - make distclean > + - git clean -xfd > >- ./autogen.sh --with-session-info=console-kit --with-init-script=redhat >- make >- make install >- make uninstall > - - make distclean > + - git clean -xfd > > -configure-without-optionals: > - script: >- ./autogen.sh --with-gtk=no --with-session-info=none >- make >- make install >- make uninstall > - - make distclean > + - git clean -xfd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH linux vdagent] Advertise VD_AGENT_CAP_GRAPHICS_DEVICE_INFO
> > Since we now support the graphics device info message, advertise this > fact when sending capabilities to the server. > > Signed-off-by: Jonathon Jongsma Acked-by: Frediano Ziglio > --- > src/vdagentd/vdagentd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c > index f80f48b..72a3e13 100644 > --- a/src/vdagentd/vdagentd.c > +++ b/src/vdagentd/vdagentd.c > @@ -132,6 +132,7 @@ static void send_capabilities(struct vdagent_virtio_port > *vport, > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF); > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD); > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC); > +VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GRAPHICS_DEVICE_INFO); > virtio_msg_uint32_to_le((uint8_t *)caps, size, 0); > > vdagent_virtio_port_write(vport, VDP_CLIENT_PORT, ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
