Re: [Spice-devel] [vdagentd-linux] vdagentd: Do not call session_info_is_user() with invalid session_info
Ack
On Tue, 2017-05-16 at 19:21 +0200, Victor Toso wrote:
> From: Victor Toso
>
> If we pass -X command line option which disables
> console-kit/systemd-logind integration, we will have the following
> critical being issued:
>
> CRITICAL **: session_info_is_user: assertion 'si != NULL' failed
>
> This patch avoid it.
>
> Signed-off-by: Victor Toso
> ---
> src/vdagentd/vdagentd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 800a09c..7ffb890 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -786,7 +786,9 @@ static void
> update_active_session_connection(struct udscs_connection *new_conn)
> if (debug)
> syslog(LOG_DEBUG, "%p is now the active session",
> new_conn);
>
> -if (active_session_conn && !session_info_is_user(session_info))
> {
> +if (active_session_conn &&
> +session_info != NULL &&
> +!session_info_is_user(session_info)) {
> if (debug)
> syslog(LOG_DEBUG, "New session agent does not belong to
> user: "
> "disabling file-xfer");
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 3/5] Remove warning about unused variable when building on macOS
On Wed, 2017-05-17 at 07:08 +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> On macOS, neither of the two cases implemented in set_mouse_accel
> applies.
hmm, it should be on wayland too.
> We get the following eror message:
>
> CC spice-widget.lo
> spice-widget.c:944:26: error: unused variable 'd' [-Werror,-Wunused-
> variable]
> SpiceDisplayPrivate *d = display->priv;
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-widget.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 8203d55..5542e85 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -941,9 +941,8 @@ static void update_keyboard_grab(SpiceDisplay
> *display)
>
> static void set_mouse_accel(SpiceDisplay *display, gboolean
> enabled)
Don't you get a warning about unused parameters?
> {
> -SpiceDisplayPrivate *d = display->priv;
> -
> #if defined GDK_WINDOWING_X11
> +SpiceDisplayPrivate *d = display->priv;
> GdkWindow *w =
> GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
>
> if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
> @@ -965,6 +964,7 @@ static void set_mouse_accel(SpiceDisplay
> *display, gboolean enabled)
>d->x11_accel_numerator, d-
> >x11_accel_denominator, d->x11_threshold);
> }
> #elif defined GDK_WINDOWING_WIN32
> +SpiceDisplayPrivate *d = display->priv;
> if (enabled) {
> g_return_if_fail(SystemParametersInfo(SPI_SETMOUSE, 0, &d-
> >win_mouse, 0));
> g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0,
> (PVOID)(INT_PTR)d->win_mouse_speed, 0));
Ack,
Pavel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 5/5] Use SPICE_ALIGNED_CAST to silence warning with ucontext on macOS
>
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
Lot of hunks in this patch should be merged in previous patches.
Frediano
> ---
> configure.ac| 3 ++-
> src/channel-cursor.c| 6 +++---
> src/channel-display-mjpeg.c | 2 +-
> src/continuation.h | 6 --
> src/decode-glz-tmpl.c | 2 +-
> src/spice-channel.c | 10 +-
> 6 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index ecab365..8b433ba 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -481,7 +481,8 @@ if test "$with_coroutine" = "auto"; then
>if test "$os_win32" = "yes"; then
> with_coroutine=winfiber
>elif test "$os_mac" = "yes"; then
> -with_coroutine=gthread
> +with_coroutine=ucontext
> +AC_DEFINE([_XOPEN_SOURCE], [1], [Define _XOPEN_SOURCE on macOS for
> ucontext])
>else
> with_coroutine=ucontext
>fi
> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> index 50de5ce..650d408 100644
> --- a/src/channel-cursor.c
> +++ b/src/channel-cursor.c
> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> memcpy(cursor->data, data, size);
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> -if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
> 0xff) {
> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32 *, data) + i) ==
> 0xff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> cursor->data[i] |= (pix_mask ? 0 : 0xff00);
> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> case SPICE_CURSOR_TYPE_COLOR16:
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> -pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
> +pix = *(SPICE_ALIGNED_CAST(guint16 *, data) + i);
> if (pix_mask && pix == 0x7fff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> @@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4),
> i);
> int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] &
> 0xf0) >> 4);
> -pix = *(SPICE_UNALIGNED_CAST(uint32_t*,(data + size)) + idx);
> +pix = *(SPICE_UNALIGNED_CAST(uint32_t *, (data + size)) + idx);
> if (pix_mask && pix == 0xff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 17c0f4f..ee33b01 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> #ifndef JCS_EXTENSIONS
> {
> uint8_t *s = lines[0];
> -uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
> +uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *, s);
>
> if (back_compat) {
> for (unsigned int j = lines_read * width; j > 0; ) {
> diff --git a/src/continuation.h b/src/continuation.h
> index 675a257..d1fd137 100644
> --- a/src/continuation.h
> +++ b/src/continuation.h
> @@ -21,6 +21,7 @@
> #ifndef _CONTINUATION_H_
> #define _CONTINUATION_H_
>
> +#include "spice-common.h"
> #include
> #include
> #include
> @@ -48,8 +49,9 @@ int cc_release(struct continuation *cc);
> int cc_swap(struct continuation *from, struct continuation *to);
>
> #define offset_of(type, member) ((unsigned long)(&((type *)0)->member))
> -#define container_of(obj, type, member) \
> -(type *)(((char *)obj) - offset_of(type, member))
> +#define container_of(obj, type, member) \
> +SPICE_ALIGNED_CAST(type *, \
> + (((char *)obj) - offset_of(type, member)))
>
> #endif
> /*
> diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
> index 7695a28..76d832c 100644
> --- a/src/decode-glz-tmpl.c
> +++ b/src/decode-glz-tmpl.c
> @@ -178,7 +178,7 @@ static size_t FNAME(decode)(SpiceGlzDecoderWindow
> *window,
> uint64_t image_id, SpicePalette *plt)
> {
> uint8_t *ip = in_buf;
> -OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *,out_buf);
> +OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *, out_buf);
> OUT_PIXEL*op = out_pix_buf;
> OUT_PIXEL*op_limit = out_pix_buf + size;
>
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 88d0567..89934a0 10064
Re: [Spice-devel] [PATCH spice-gtk v3 5/5] Use SPICE_ALIGNED_CAST to silence warning with ucontext on macOS
On Wed, May 17, 2017 at 07:08:26AM +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin
>
> Signed-off-by: Christophe de Dinechin
> ---
> configure.ac| 3 ++-
> src/channel-cursor.c| 6 +++---
> src/channel-display-mjpeg.c | 2 +-
> src/continuation.h | 6 --
> src/decode-glz-tmpl.c | 2 +-
> src/spice-channel.c | 10 +-
> 6 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index ecab365..8b433ba 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -481,7 +481,8 @@ if test "$with_coroutine" = "auto"; then
>if test "$os_win32" = "yes"; then
> with_coroutine=winfiber
>elif test "$os_mac" = "yes"; then
> -with_coroutine=gthread
> +with_coroutine=ucontext
> +AC_DEFINE([_XOPEN_SOURCE], [1], [Define _XOPEN_SOURCE on macOS for
> ucontext])
>else
> with_coroutine=ucontext
>fi
> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> index 50de5ce..650d408 100644
> --- a/src/channel-cursor.c
> +++ b/src/channel-cursor.c
> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> memcpy(cursor->data, data, size);
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> -if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
> 0xff) {
> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32 *, data) + i) ==
> 0xff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> cursor->data[i] |= (pix_mask ? 0 : 0xff00);
> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> case SPICE_CURSOR_TYPE_COLOR16:
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> -pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
> +pix = *(SPICE_ALIGNED_CAST(guint16 *, data) + i);
> if (pix_mask && pix == 0x7fff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> @@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4), i);
> int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] &
> 0xf0) >> 4);
> -pix = *(SPICE_UNALIGNED_CAST(uint32_t*,(data + size)) + idx);
> +pix = *(SPICE_UNALIGNED_CAST(uint32_t *, (data + size)) + idx);
> if (pix_mask && pix == 0xff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 17c0f4f..ee33b01 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> #ifndef JCS_EXTENSIONS
> {
> uint8_t *s = lines[0];
> -uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
> +uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *, s);
>
> if (back_compat) {
> for (unsigned int j = lines_read * width; j > 0; ) {
While these whitespace changes make sense, they are not related to the
commit message.
> diff --git a/src/continuation.h b/src/continuation.h
> index 675a257..d1fd137 100644
> --- a/src/continuation.h
> +++ b/src/continuation.h
> @@ -21,6 +21,7 @@
> #ifndef _CONTINUATION_H_
> #define _CONTINUATION_H_
>
> +#include "spice-common.h"
> #include
> #include
> #include
> @@ -48,8 +49,9 @@ int cc_release(struct continuation *cc);
> int cc_swap(struct continuation *from, struct continuation *to);
>
> #define offset_of(type, member) ((unsigned long)(&((type *)0)->member))
> -#define container_of(obj, type, member) \
> -(type *)(((char *)obj) - offset_of(type, member))
> +#define container_of(obj, type, member) \
> +SPICE_ALIGNED_CAST(type *, \
> + (((char *)obj) - offset_of(type, member)))
>
> #endif
> /*
Yep, makes sense.
> diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
> index 7695a28..76d832c 100644
> --- a/src/decode-glz-tmpl.c
> +++ b/src/decode-glz-tmpl.c
> @@ -178,7 +178,7 @@ static size_t FNAME(decode)(SpiceGlzDecoderWindow *window,
> uint64_t image_id, SpicePalette *plt)
> {
> uint8_t *ip = in_buf;
> -OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *,out_buf);
> +OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *, out_buf);
> OUT_PIXEL*op = out_pix_buf;
> OUT_PIXEL*op_limit = out_pix_buf + size;
>
More unr
Re: [Spice-devel] [PATCH spice-gtk v3 4/5] Add check for macOS, disable ucontext on macOS (deprecated)
On Wed, May 17, 2017 at 07:08:25AM +0200, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > configure.ac | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 74b5811..ecab365 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -62,6 +62,18 @@ esac > AC_MSG_RESULT([$os_win32]) > AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) > > +AC_MSG_CHECKING([for native macOS]) > +case "$host_os" in > + *darwin*) > +os_mac=yes > +;; > + *) > +os_mac=no > +;; > +esac > +AC_MSG_RESULT([$os_mac]) > +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) > + > AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) > AC_CHECK_HEADERS([termios.h]) > AC_CHECK_HEADERS([epoxy/egl.h], > @@ -468,6 +480,8 @@ esac > if test "$with_coroutine" = "auto"; then >if test "$os_win32" = "yes"; then > with_coroutine=winfiber > + elif test "$os_mac" = "yes"; then > +with_coroutine=gthread >else > with_coroutine=ucontext >fi Surely this patch should be dropped, given patch 5 fixes the warnings now Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 4/5] Add check for macOS, disable ucontext on macOS (deprecated)
On Wed, 2017-05-17 at 07:08 +0200, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > configure.ac | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 74b5811..ecab365 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -62,6 +62,18 @@ esac > AC_MSG_RESULT([$os_win32]) > AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) > > +AC_MSG_CHECKING([for native macOS]) > +case "$host_os" in > + *darwin*) > +os_mac=yes > +;; > + *) > +os_mac=no > +;; > +esac > +AC_MSG_RESULT([$os_mac]) > +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) > + Although I see its benefits I would merge it with the previous switch > AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) > AC_CHECK_HEADERS([termios.h]) > AC_CHECK_HEADERS([epoxy/egl.h], > @@ -468,6 +480,8 @@ esac > if test "$with_coroutine" = "auto"; then >if test "$os_win32" = "yes"; then > with_coroutine=winfiber > + elif test "$os_mac" = "yes"; then > +with_coroutine=gthread >else > with_coroutine=ucontext >fi ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
>
> From: Christophe de Dinechin
>
> For example, something like this:
> uint8_t *p8;
> uint32_t *p32 = (uint32_t *) p8;
>
> generates a warning like this:
> spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
> *') to
> 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1
> to
> 4 [-Werror,-Wcast-align]
>
> The warning indicates that we end up with a pointer to data that
> should be 4-byte aligned, but its value may be misaligned. On x86,
> this does not make much of a difference, except a relatively minor
> performance penalty. However, on platforms such as ARM, misaligned
> accesses are emulated by the kernel, and support for them is optional.
> So we may end up with a fault.
>
> The intent of the fix here is to make it easy to identify and rework
> places where actual mis-alignment occurs. Wherever casts raise the warning,
> they are replaced with a macro:
>
> - SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
> we believe the resulting pointer is aligned. If it is not, a warning will
> be issued.
>
> - SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates that
> we believe the resulting pointer is not always aligned
>
> Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
> to improve performance, e.g. by using memcpy.
>
> There are normally no warnings for SPICE_UNALIGNED_CAST, but it is possible
> to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
> Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
>
> Signed-off-by: Christophe de Dinechin
> ---
> spice-common| 2 +-
> src/channel-cursor.c| 6 +++---
> src/channel-display-mjpeg.c | 2 +-
> src/decode-glz-tmpl.c | 2 +-
> src/spice-channel.c | 14 +-
> 5 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/spice-common b/spice-common
> index af682b1..1239c82 16
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
> +Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> index 609243b..50de5ce 100644
> --- a/src/channel-cursor.c
> +++ b/src/channel-cursor.c
> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> memcpy(cursor->data, data, size);
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> -if (pix_mask && *((guint32*)data + i) == 0xff) {
> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
> 0xff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> cursor->data[i] |= (pix_mask ? 0 : 0xff00);
> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> case SPICE_CURSOR_TYPE_COLOR16:
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size, i);
> -pix = *((guint16*)data + i);
> +pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
> if (pix_mask && pix == 0x7fff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
If these 2 hits you are going to have a message for every single pixel
in the image, potentially millions.
I'm still convinced, like in this case, that if we are sure that data
should be aligned no check and warning should be done.
I'm really sure no architecture on hearth can do check and print in
a single machine instruction. Actually I don't know any architecture
where a single check like this can be done in a single instruction.
> @@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
> SpiceCursor *scursor)
> for (i = 0; i < hdr->width * hdr->height; i++) {
> pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4),
> i);
> int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] &
> 0xf0) >> 4);
> -pix = *((uint32_t*)(data + size) + idx);
> +pix = *(SPICE_UNALIGNED_CAST(uint32_t*,(data + size)) + idx);
> if (pix_mask && pix == 0xff) {
> cursor->data[i] = get_pix_hack(i, hdr->width);
> } else {
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 3ae9d21..17c0f4f 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> #ifndef JCS_EXTENSIONS
> {
> uint8_t *s = lines[0];
> -uint32_t *d = (uint32_t *)s;
> +uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
>
> if (back_compat) {
> for (
Re: [Spice-devel] [PATCH spice-gtk v3 2/5] Avoid warning about snprintf on non-Linux platforms
> > From: Christophe de Dinechin > > Without #include , calls to snprintf in the file > cause a warning. The file is left aside on purpose, > since src/usbutil.c may be compiled on Windows where this > file does not exist. > > Signed-off-by: Christophe de Dinechin Acked-by: Frediano Ziglio > --- > src/usbutil.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/usbutil.c b/src/usbutil.c > index b68a2e1..e96ab11 100644 > --- a/src/usbutil.c > +++ b/src/usbutil.c > @@ -27,8 +27,8 @@ > #include > > #ifdef USE_USBREDIR > -#ifdef __linux__ > #include > +#ifdef __linux__ > #include > #include > #ifndef major /* major and minor macros were moved to sys/sysmacros.h from > sys/types.h */ Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 3/5] Remove warning about unused variable when building on macOS
>
> From: Christophe de Dinechin
>
> On macOS, neither of the two cases implemented in set_mouse_accel applies.
> We get the following eror message:
>
> CC spice-widget.lo
> spice-widget.c:944:26: error: unused variable 'd' [-Werror,-Wunused-variable]
> SpiceDisplayPrivate *d = display->priv;
>
> Signed-off-by: Christophe de Dinechin
> ---
> src/spice-widget.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 8203d55..5542e85 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -941,9 +941,8 @@ static void update_keyboard_grab(SpiceDisplay *display)
>
> static void set_mouse_accel(SpiceDisplay *display, gboolean enabled)
> {
> -SpiceDisplayPrivate *d = display->priv;
> -
> #if defined GDK_WINDOWING_X11
> +SpiceDisplayPrivate *d = display->priv;
> GdkWindow *w = GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
>
> if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
> @@ -965,6 +964,7 @@ static void set_mouse_accel(SpiceDisplay *display,
> gboolean enabled)
>d->x11_accel_numerator, d->x11_accel_denominator,
>d->x11_threshold);
> }
> #elif defined GDK_WINDOWING_WIN32
> +SpiceDisplayPrivate *d = display->priv;
> if (enabled) {
> g_return_if_fail(SystemParametersInfo(SPI_SETMOUSE, 0,
> &d->win_mouse, 0));
> g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0,
> (PVOID)(INT_PTR)d->win_mouse_speed, 0));
A TODO comment on the #else case would be helpful.
I remember was agreed to add one.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] OT Re: [spice-gtk] glz: Remove unused DECODE_TO_SAME array
>
> Hi,
>
> On Tue, May 16, 2017 at 12:29:16PM -0400, Frediano Ziglio wrote:
> > My script says (not checked):
>
> Can you share it? :)
>
This is the spice-server version
https://cgit.freedesktop.org/~fziglio/script-utils/tree/spice-server/unused.pl.
It's quite manual at the moment, I usually copy and change for the
different projects. In case of spice-gtk I changed the export file
and reading code and list of paths.
Basically take all global symbols, remove ones exported or with relocations
and print (for unused).
> > Unused functions:
> > canvas_create
> > g_cclosure_user_marshal_VOID__OBJECT_OBJECT
> > spice_display_get_pixbuf
> > spice_display_key_event_get_type
> > spice_display_mouse_ungrab
> > spice_display_new
>
> Does your script check for unused *exported* functions too? Although
> this is on src/map-file it isn't used in spicy or virt-viewer at least
> (only spice_display_new_with_monitor() is used)
>
No, I assume exported means used by some external tools (so cannot check)
and part of the ABI (so better to retain). But can be changed if they are
used just by known tools (spicy?).
> > spice_display_send_keys
> > spice_grab_sequence_get_type
> > spice_grab_sequence_new
> > spice_msg_in_hexdump
> > spice_msg_out_hexdump
> > spice_msg_out_ref
> > spice_session_set_main_channel
> > vnc_display_keyval_free_entries
> > vnc_display_keyval_from_keycode
> > vnc_display_keyval_set_entries
> > Can be static functions:
> > coroutine_release
> > coroutine_swap
> > spice_desktop_integration_get_type
> > spice_gstaudio_get_type
> > spice_gtk_session_get_type
> > spice_image_compress_get_type
> > spice_msg_in_new
> > spice_msg_in_sub_new
> > spice_pulse_get_type
> > spice_session_get_shared_dir
> > spice_session_set_shared_dir
> > spice_usb_device_get_busnum
> > spice_usb_device_get_devaddr
> > spice_usb_device_widget_get_type
> > spice_vmc_output_stream_get_type
> > spice_vmc_stream_get_type
> >
> > Frediano
>
> Would be nice to make the ones that can be static before the next
> release and checking what can be removed/deprecated from the non used
> ones..
>
Note that the scripts just uses object file from current compiled
code, so for instance maybe compiling with different options (or
for different target) can lead different results and they should
be checked manually.
>
> >
> > - Original Message -
> > > From: "Christophe Fergeau"
> > > To: spice-devel@lists.freedesktop.org
> > > Sent: Tuesday, May 16, 2017 4:12:42 PM
> > > Subject: [Spice-devel] [spice-gtk] glz: Remove unused DECODE_TO_SAME
> > > array
> > >
> > > This also removes the generation of unneeded _decode methods once this
> > > array is removed (this causes some warnings otherwise).
> > >
> > > DECODE_TO_SAME was used in the old client, but was never used when this
> > > code was moved over to spice-gtk.
> > >
> > > Signed-off-by: Christophe Fergeau
> > > ---
> > > src/decode-glz.c | 21 -
> > > 1 file changed, 21 deletions(-)
> > >
> > > diff --git a/src/decode-glz.c b/src/decode-glz.c
> > > index d5b72ab5..27765393 100644
> > > --- a/src/decode-glz.c
> > > +++ b/src/decode-glz.c
> > > @@ -238,9 +238,6 @@ typedef uint16_t rgb16_pixel_t;
> > > #undef ATTR_PACKED
> > >
> > > #define LZ_PLT
> > > -#include "decode-glz-tmpl.c"
> > > -
> > > -#define LZ_PLT
> > > #define PLT8
> > > #define TO_RGB32
> > > #include "decode-glz-tmpl.c"
> > > @@ -267,14 +264,9 @@ typedef uint16_t rgb16_pixel_t;
> > >
> > >
> > > #define LZ_RGB16
> > > -#include "decode-glz-tmpl.c"
> > > -#define LZ_RGB16
> > > #define TO_RGB32
> > > #include "decode-glz-tmpl.c"
> > >
> > > -#define LZ_RGB24
> > > -#include "decode-glz-tmpl.c"
> > > -
> > > #define LZ_RGB32
> > > #include "decode-glz-tmpl.c"
> > >
> > > @@ -302,19 +294,6 @@ const decode_function DECODE_TO_RGB32[] = {
> > > glz_rgb32_decode
> > > };
> > >
> > > -const decode_function DECODE_TO_SAME[] = {
> > > -NULL,
> > > -glz_plt_decode,
> > > -glz_plt_decode,
> > > -glz_plt_decode,
> > > -glz_plt_decode,
> > > -glz_plt_decode,
> > > -glz_rgb16_decode,
> > > -glz_rgb24_decode,
> > > -glz_rgb32_decode,
> > > -glz_rgb32_decode
> > > -};
> > > -
> > > static uint32_t decode_32(GlibGlzDecoder *d)
> > > {
> > > uint32_t word = 0;
> >
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] Using spice in an ESXI environment
I was pointed to the spice-space page by someone after discussing problems with VNC. I was under the impression spice only worked with KVM, I am working with an ESXI based VM environment though and looking for a promising VDI solution. Is it possible to use spice in this set up? ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v4 3/4] Avoid warning about snprintf on non-Linux platforms
From: Christophe de Dinechin Without #include , calls to snprintf in the file cause a warning. The file is left aside on purpose, since src/usbutil.c may be compiled on Windows where this file does not exist. Signed-off-by: Christophe de Dinechin --- src/usbutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/usbutil.c b/src/usbutil.c index b68a2e1..e96ab11 100644 --- a/src/usbutil.c +++ b/src/usbutil.c @@ -27,8 +27,8 @@ #include #ifdef USE_USBREDIR -#ifdef __linux__ #include +#ifdef __linux__ #include #include #ifndef major /* major and minor macros were moved to sys/sysmacros.h from sys/types.h */ -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v4 2/4] Avoid clang warnings on casts with stricter alignment requirements
From: Christophe de Dinechin
For example, something like this:
uint8_t *p8;
uint32_t *p32 = (uint32_t *) p8;
generates a warning like this:
spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char *')
to
'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to
4 [-Werror,-Wcast-align]
The warning indicates that we end up with a pointer to data that
should be 4-byte aligned, but its value may be misaligned. On x86,
this does not make much of a difference, except a relatively minor
performance penalty. However, on platforms such as ARM, misaligned
accesses are emulated by the kernel, and support for them is optional.
So we may end up with a fault.
The intent of the fix here is to make it easy to identify and rework
places where actual mis-alignment occurs. Wherever casts raise the warning,
they are replaced with a macro:
- SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
we believe the resulting pointer is aligned. If it is not, a warning will
be issued.
- SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates that
we believe the resulting pointer is not always aligned
Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
to improve performance, e.g. by using memcpy.
There are normally no warnings for SPICE_UNALIGNED_CAST, but it is possible
to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
Signed-off-by: Christophe de Dinechin
---
spice-common| 2 +-
src/channel-cursor.c| 6 +++---
src/channel-display-mjpeg.c | 2 +-
src/continuation.h | 6 --
src/decode-glz-tmpl.c | 2 +-
src/spice-channel.c | 14 +-
6 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/spice-common b/spice-common
index af682b1..1239c82 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
+Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
diff --git a/src/channel-cursor.c b/src/channel-cursor.c
index 609243b..650d408 100644
--- a/src/channel-cursor.c
+++ b/src/channel-cursor.c
@@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
SpiceCursor *scursor)
memcpy(cursor->data, data, size);
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-if (pix_mask && *((guint32*)data + i) == 0xff) {
+if (pix_mask && *(SPICE_ALIGNED_CAST(guint32 *, data) + i) ==
0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
cursor->data[i] |= (pix_mask ? 0 : 0xff00);
@@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
SpiceCursor *scursor)
case SPICE_CURSOR_TYPE_COLOR16:
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-pix = *((guint16*)data + i);
+pix = *(SPICE_ALIGNED_CAST(guint16 *, data) + i);
if (pix_mask && pix == 0x7fff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
@@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
SpiceCursor *scursor)
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4), i);
int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] & 0xf0)
>> 4);
-pix = *((uint32_t*)(data + size) + idx);
+pix = *(SPICE_UNALIGNED_CAST(uint32_t *, (data + size)) + idx);
if (pix_mask && pix == 0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ae9d21..ee33b01 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
#ifndef JCS_EXTENSIONS
{
uint8_t *s = lines[0];
-uint32_t *d = (uint32_t *)s;
+uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *, s);
if (back_compat) {
for (unsigned int j = lines_read * width; j > 0; ) {
diff --git a/src/continuation.h b/src/continuation.h
index 675a257..d1fd137 100644
--- a/src/continuation.h
+++ b/src/continuation.h
@@ -21,6 +21,7 @@
#ifndef _CONTINUATION_H_
#define _CONTINUATION_H_
+#include "spice-common.h"
#include
#include
#include
@@ -48,8 +49,9 @@ int cc_release(struct continuation *cc);
int cc_swap(struct continuation *from, struct continuation *to);
#define offset_of(type, member) ((unsigned long)(&((type *)0)->member))
-#define container_of(obj, type, member) \
-(type *)(((char *)obj) - offset_of(type, member))
+#define containe
[Spice-devel] [PATCH spice-gtk v4 0/4] Repair macOS builds for spice-gtk
From: Christophe de Dinechin This patch set contains various fixes that repair macOS builds for spice-gtk (and presumably clang builds as a side effect), notably: - Fixes for clang-specific warnings, notably on alignment - Addition of macOS detection in configure script - A couple minor portablity fixes This requires an update to spice-common, see https://lists.freedesktop.org/archives/spice-devel/2017-May/037505.html. The reference to submodule in this patch serie is updated with a reference that can be fetched from https://github.com/c3d/spice-common.git. Version 2 takes into account comments by Frediano Ziglio and Pavel Grunt, specifically: - Clarify purpose and behavior of macros in patch commit message - Change one case to 'unaligned' - Improve way to avoid 'unused variable' warning Acked by: Christophe Fergeau Version 3 takes into account comments made by Christophe Fergeau in his ack message, specifically: - Improve the log message description of SPICE_(UN)ALIGNED_CAST. Hopefully the new one is better. - Make spaces after commas consistent In addition, v3 re-enables ucontext on macOS, following comments that this is the right thing to do for performance. This required the elimination of one more alignment warning. Version 4 takes into account comments made by Pavel and Frediano, specifically: - Reorder the previous iteration to group things more logically - Add a TODO comment for macOS in set_mouse_accel Christophe de Dinechin (4): Add check for macOS Avoid clang warnings on casts with stricter alignment requirements Avoid warning about snprintf on non-Linux platforms Remove warning about unused variable when building on macOS configure.ac| 15 +++ spice-common| 2 +- src/channel-cursor.c| 6 +++--- src/channel-display-mjpeg.c | 2 +- src/continuation.h | 6 -- src/decode-glz-tmpl.c | 2 +- src/spice-channel.c | 14 +- src/spice-widget.c | 7 --- src/usbutil.c | 2 +- 9 files changed, 39 insertions(+), 17 deletions(-) -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v4 4/4] Remove warning about unused variable when building on macOS
From: Christophe de Dinechin
On macOS, neither of the two cases implemented in set_mouse_accel applies.
We get the following eror message:
CC spice-widget.lo
spice-widget.c:944:26: error: unused variable 'd' [-Werror,-Wunused-variable]
SpiceDisplayPrivate *d = display->priv;
Signed-off-by: Christophe de Dinechin
---
src/spice-widget.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/spice-widget.c b/src/spice-widget.c
index 8203d55..3bb7343 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -941,9 +941,8 @@ static void update_keyboard_grab(SpiceDisplay *display)
static void set_mouse_accel(SpiceDisplay *display, gboolean enabled)
{
-SpiceDisplayPrivate *d = display->priv;
-
#if defined GDK_WINDOWING_X11
+SpiceDisplayPrivate *d = display->priv;
GdkWindow *w = GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
@@ -965,6 +964,7 @@ static void set_mouse_accel(SpiceDisplay *display, gboolean
enabled)
d->x11_accel_numerator, d->x11_accel_denominator,
d->x11_threshold);
}
#elif defined GDK_WINDOWING_WIN32
+SpiceDisplayPrivate *d = display->priv;
if (enabled) {
g_return_if_fail(SystemParametersInfo(SPI_SETMOUSE, 0, &d->win_mouse,
0));
g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0,
(PVOID)(INT_PTR)d->win_mouse_speed, 0));
@@ -976,6 +976,7 @@ static void set_mouse_accel(SpiceDisplay *display, gboolean
enabled)
g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0, (PVOID)10,
SPIF_SENDCHANGE)); // default
}
#else
+/* TODO: Add mouse accelaration for macOS */
g_warning("Mouse acceleration code missing for your platform");
#endif
}
@@ -1616,7 +1617,7 @@ static gboolean key_event(GtkWidget *widget, GdkEventKey
*key)
if (key->keyval == GDK_KEY_Pause
#ifdef G_OS_WIN32
-/* for some reason GDK does not fill keyval for VK_PAUSE
+/* for some reason GDK does not fill keyval for VK_PAUSE
* See https://bugzilla.gnome.org/show_bug.cgi?id=769214
*/
|| key->hardware_keycode == VK_PAUSE
--
2.11.0 (Apple Git-81)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v4 1/4] Add check for macOS
From: Christophe de Dinechin Signed-off-by: Christophe de Dinechin --- configure.ac | 15 +++ 1 file changed, 15 insertions(+) diff --git a/configure.ac b/configure.ac index ff00d73..62acafc 100644 --- a/configure.ac +++ b/configure.ac @@ -62,6 +62,18 @@ esac AC_MSG_RESULT([$os_win32]) AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) +AC_MSG_CHECKING([for native macOS]) +case "$host_os" in + *darwin*) +os_mac=yes +;; + *) +os_mac=no +;; +esac +AC_MSG_RESULT([$os_mac]) +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) + AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) AC_CHECK_HEADERS([termios.h]) AC_CHECK_HEADERS([epoxy/egl.h], @@ -468,6 +480,9 @@ esac if test "$with_coroutine" = "auto"; then if test "$os_win32" = "yes"; then with_coroutine=winfiber + elif test "$os_mac" = "yes"; then +with_coroutine=ucontext +AC_DEFINE([_XOPEN_SOURCE], [1], [Define _XOPEN_SOURCE on macOS for ucontext]) else with_coroutine=ucontext fi -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
On Wed, May 17, 2017 at 03:44:48AM -0400, Frediano Ziglio wrote: > If these 2 hits you are going to have a message for every single pixel > in the image, potentially millions. > I'm still convinced, like in this case, that if we are sure that data > should be aligned no check and warning should be done. > I'm really sure no architecture on hearth can do check and print in > a single machine instruction. Actually I don't know any architecture > where a single check like this can be done in a single instruction. I was keeping this comment for the review of the spice-common patches introducing the macros, but I indeed wanted to ask about the performance impact of the macros, and whether we really need to always do the checks, or if we should only do these when debug mode is enabled at build time. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 3/5] Remove warning about unused variable when building on macOS
On Wed, May 17, 2017 at 03:47:48AM -0400, Frediano Ziglio wrote:
> >
> > From: Christophe de Dinechin
> >
> > On macOS, neither of the two cases implemented in set_mouse_accel applies.
> > We get the following eror message:
> >
> > CC spice-widget.lo
> > spice-widget.c:944:26: error: unused variable 'd'
> > [-Werror,-Wunused-variable]
> > SpiceDisplayPrivate *d = display->priv;
> >
> > Signed-off-by: Christophe de Dinechin
> > ---
> > src/spice-widget.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 8203d55..5542e85 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -941,9 +941,8 @@ static void update_keyboard_grab(SpiceDisplay *display)
> >
> > static void set_mouse_accel(SpiceDisplay *display, gboolean enabled)
> > {
> > -SpiceDisplayPrivate *d = display->priv;
> > -
> > #if defined GDK_WINDOWING_X11
> > +SpiceDisplayPrivate *d = display->priv;
> > GdkWindow *w = GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
> >
> > if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
> > @@ -965,6 +964,7 @@ static void set_mouse_accel(SpiceDisplay *display,
> > gboolean enabled)
> >d->x11_accel_numerator, d->x11_accel_denominator,
> >d->x11_threshold);
> > }
> > #elif defined GDK_WINDOWING_WIN32
> > +SpiceDisplayPrivate *d = display->priv;
> > if (enabled) {
> > g_return_if_fail(SystemParametersInfo(SPI_SETMOUSE, 0,
> > &d->win_mouse, 0));
> > g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0,
> > (PVOID)(INT_PTR)d->win_mouse_speed, 0));
>
> A TODO comment on the #else case would be helpful.
> I remember was agreed to add one.
Honestly, I would just keep the compile-time warning until we know what
to do with this code on osx.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 3/5] Remove warning about unused variable when building on macOS
> On 17 May 2017, at 11:24, Christophe Fergeau wrote:
>
> On Wed, May 17, 2017 at 03:47:48AM -0400, Frediano Ziglio wrote:
>>>
>>> From: Christophe de Dinechin
>>>
>>> On macOS, neither of the two cases implemented in set_mouse_accel applies.
>>> We get the following eror message:
>>>
>>> CC spice-widget.lo
>>> spice-widget.c:944:26: error: unused variable 'd'
>>> [-Werror,-Wunused-variable]
>>>SpiceDisplayPrivate *d = display->priv;
>>>
>>> Signed-off-by: Christophe de Dinechin
>>> ---
>>> src/spice-widget.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/spice-widget.c b/src/spice-widget.c
>>> index 8203d55..5542e85 100644
>>> --- a/src/spice-widget.c
>>> +++ b/src/spice-widget.c
>>> @@ -941,9 +941,8 @@ static void update_keyboard_grab(SpiceDisplay *display)
>>>
>>> static void set_mouse_accel(SpiceDisplay *display, gboolean enabled)
>>> {
>>> -SpiceDisplayPrivate *d = display->priv;
>>> -
>>> #if defined GDK_WINDOWING_X11
>>> +SpiceDisplayPrivate *d = display->priv;
>>> GdkWindow *w = GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
>>>
>>> if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
>>> @@ -965,6 +964,7 @@ static void set_mouse_accel(SpiceDisplay *display,
>>> gboolean enabled)
>>> d->x11_accel_numerator, d->x11_accel_denominator,
>>> d->x11_threshold);
>>> }
>>> #elif defined GDK_WINDOWING_WIN32
>>> +SpiceDisplayPrivate *d = display->priv;
>>> if (enabled) {
>>> g_return_if_fail(SystemParametersInfo(SPI_SETMOUSE, 0,
>>> &d->win_mouse, 0));
>>> g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0,
>>> (PVOID)(INT_PTR)d->win_mouse_speed, 0));
>>
>> A TODO comment on the #else case would be helpful.
>> I remember was agreed to add one.
>
> Honestly, I would just keep the compile-time warning until we know what
> to do with this code on osx.
I did not remove the compile-time warning. I added the comment. But I did not
find an obvious way to control acceleration in Sierra.
I don’t see much of an adverse side effect to not having this code. How could I
test it?
Christophe
>
> Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
> On 17 May 2017, at 09:44, Frediano Ziglio wrote:
>
>>
>> From: Christophe de Dinechin
>>
>> For example, something like this:
>>uint8_t *p8;
>>uint32_t *p32 = (uint32_t *) p8;
>>
>> generates a warning like this:
>> spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
>> *') to
>> 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1
>> to
>> 4 [-Werror,-Wcast-align]
>>
>> The warning indicates that we end up with a pointer to data that
>> should be 4-byte aligned, but its value may be misaligned. On x86,
>> this does not make much of a difference, except a relatively minor
>> performance penalty. However, on platforms such as ARM, misaligned
>> accesses are emulated by the kernel, and support for them is optional.
>> So we may end up with a fault.
>>
>> The intent of the fix here is to make it easy to identify and rework
>> places where actual mis-alignment occurs. Wherever casts raise the warning,
>> they are replaced with a macro:
>>
>> - SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
>> we believe the resulting pointer is aligned. If it is not, a warning will
>> be issued.
>>
>> - SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates that
>> we believe the resulting pointer is not always aligned
>>
>> Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
>> to improve performance, e.g. by using memcpy.
>>
>> There are normally no warnings for SPICE_UNALIGNED_CAST, but it is possible
>> to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
>> Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
>>
>> Signed-off-by: Christophe de Dinechin
>> ---
>> spice-common| 2 +-
>> src/channel-cursor.c| 6 +++---
>> src/channel-display-mjpeg.c | 2 +-
>> src/decode-glz-tmpl.c | 2 +-
>> src/spice-channel.c | 14 +-
>> 5 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/spice-common b/spice-common
>> index af682b1..1239c82 16
>> --- a/spice-common
>> +++ b/spice-common
>> @@ -1 +1 @@
>> -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
>> +Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
>> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
>> index 609243b..50de5ce 100644
>> --- a/src/channel-cursor.c
>> +++ b/src/channel-cursor.c
>> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
>> SpiceCursor *scursor)
>> memcpy(cursor->data, data, size);
>> for (i = 0; i < hdr->width * hdr->height; i++) {
>> pix_mask = get_pix_mask(data, size, i);
>> -if (pix_mask && *((guint32*)data + i) == 0xff) {
>> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
>> 0xff) {
>> cursor->data[i] = get_pix_hack(i, hdr->width);
>> } else {
>> cursor->data[i] |= (pix_mask ? 0 : 0xff00);
>> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
>> SpiceCursor *scursor)
>> case SPICE_CURSOR_TYPE_COLOR16:
>> for (i = 0; i < hdr->width * hdr->height; i++) {
>> pix_mask = get_pix_mask(data, size, i);
>> -pix = *((guint16*)data + i);
>> +pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
>> if (pix_mask && pix == 0x7fff) {
>> cursor->data[i] = get_pix_hack(i, hdr->width);
>> } else {
>
> If these 2 hits you are going to have a message for every single pixel
> in the image, potentially millions.
No, you will get one message only when you enter the image.
static const char *last_loc = NULL;
if (loc != last_loc) {
last_loc = loc;
spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_WARNING, loc, __FUNCTION__,
"Misaligned access at %p, alignment %u", p, sz);
}
If it’s the same source code location, you get it once. But if you have two
such warnings, then yes, it may become verbose.
But if you get millions of calls in case of mis-alignment, it means that on
ARM, you get millions of faults into the kernel to correct the alignment, which
is surely a lot worse performance-wise than a call.
I remember someone (Pavel?) recently saying that Spice performance on ARM was
terrible. Something like this could easily explain it.
> I'm still convinced, like in this case, that if we are sure that data
> should be aligned no check and warning should be done.
Would you be OK with a check and warning under #ifndef NDEBUG (like an assert).
> I'm really sure no architecture on hearth can do check and print in
> a single machine instruction.
This is not what I said. I said that the cost in the likely case was one extra
instruction.
> Actually I don't know any architecture
> where a single check like this can be done in a single instruction.
Reduced source code: https://pastebin.com/ibZEVThA.
Machine code f
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
On Wed, May 17, 2017 at 11:45:50AM +0200, Christophe de Dinechin wrote: > > I'm really sure no architecture on hearth can do check and print in > > a single machine instruction. > > This is not what I said. I said that the cost in the likely case was one > extra instruction. > > > Actually I don't know any architecture > > where a single check like this can be done in a single instruction. > > Reduced source code: https://pastebin.com/ibZEVThA. > > Machine code for x86 with clang: https://pastebin.com/Vmtwpkhf > > The test follows the call to malloc. It’s a single “testb” instruction > followed by a never-taken conditional jump to cold-code. NB: feel free to annotate such tests with G_LIKELY/G_UNLIKELY Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
>
>
> > On 17 May 2017, at 09:44, Frediano Ziglio wrote:
> >
> >>
> >> From: Christophe de Dinechin
> >>
> >> For example, something like this:
> >>uint8_t *p8;
> >>uint32_t *p32 = (uint32_t *) p8;
> >>
> >> generates a warning like this:
> >> spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
> >> *') to
> >> 'uint32_t *' (aka 'unsigned int *') increases required alignment from
> >> 1
> >> to
> >> 4 [-Werror,-Wcast-align]
> >>
> >> The warning indicates that we end up with a pointer to data that
> >> should be 4-byte aligned, but its value may be misaligned. On x86,
> >> this does not make much of a difference, except a relatively minor
> >> performance penalty. However, on platforms such as ARM, misaligned
> >> accesses are emulated by the kernel, and support for them is optional.
> >> So we may end up with a fault.
> >>
> >> The intent of the fix here is to make it easy to identify and rework
> >> places where actual mis-alignment occurs. Wherever casts raise the
> >> warning,
> >> they are replaced with a macro:
> >>
> >> - SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
> >> we believe the resulting pointer is aligned. If it is not, a warning will
> >> be issued.
> >>
> >> - SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates
> >> that
> >> we believe the resulting pointer is not always aligned
> >>
> >> Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
> >> to improve performance, e.g. by using memcpy.
> >>
> >> There are normally no warnings for SPICE_UNALIGNED_CAST, but it is
> >> possible
> >> to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
> >> Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
> >>
> >> Signed-off-by: Christophe de Dinechin
> >> ---
> >> spice-common| 2 +-
> >> src/channel-cursor.c| 6 +++---
> >> src/channel-display-mjpeg.c | 2 +-
> >> src/decode-glz-tmpl.c | 2 +-
> >> src/spice-channel.c | 14 +-
> >> 5 files changed, 15 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/spice-common b/spice-common
> >> index af682b1..1239c82 16
> >> --- a/spice-common
> >> +++ b/spice-common
> >> @@ -1 +1 @@
> >> -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
> >> +Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
> >> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> >> index 609243b..50de5ce 100644
> >> --- a/src/channel-cursor.c
> >> +++ b/src/channel-cursor.c
> >> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel
> >> *channel,
> >> SpiceCursor *scursor)
> >> memcpy(cursor->data, data, size);
> >> for (i = 0; i < hdr->width * hdr->height; i++) {
> >> pix_mask = get_pix_mask(data, size, i);
> >> -if (pix_mask && *((guint32*)data + i) == 0xff) {
> >> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
> >> 0xff) {
> >> cursor->data[i] = get_pix_hack(i, hdr->width);
> >> } else {
> >> cursor->data[i] |= (pix_mask ? 0 : 0xff00);
> >> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel
> >> *channel,
> >> SpiceCursor *scursor)
> >> case SPICE_CURSOR_TYPE_COLOR16:
> >> for (i = 0; i < hdr->width * hdr->height; i++) {
> >> pix_mask = get_pix_mask(data, size, i);
> >> -pix = *((guint16*)data + i);
> >> +pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
> >> if (pix_mask && pix == 0x7fff) {
> >> cursor->data[i] = get_pix_hack(i, hdr->width);
> >> } else {
> >
> > If these 2 hits you are going to have a message for every single pixel
> > in the image, potentially millions.
>
> No, you will get one message only when you enter the image.
>
> static const char *last_loc = NULL;
> if (loc != last_loc) {
> last_loc = loc;
> spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_WARNING, loc,
> __FUNCTION__,
> "Misaligned access at %p, alignment %u", p, sz);
> }
>
> If it’s the same source code location, you get it once. But if you have two
> such warnings, then yes, it may become verbose.
>
> But if you get millions of calls in case of mis-alignment, it means that on
> ARM, you get millions of faults into the kernel to correct the alignment,
> which is surely a lot worse performance-wise than a call.
>
> I remember someone (Pavel?) recently saying that Spice performance on ARM was
> terrible. Something like this could easily explain it.
>
>
> > I'm still convinced, like in this case, that if we are sure that data
> > should be aligned no check and warning should be done.
>
> Would you be OK with a check and warning under #ifndef NDEBUG (like an
> assert).
>
> > I'm really sure no architecture on hearth can do check and print in
> > a single machine instruction.
Re: [Spice-devel] Using spice in an ESXI environment
Hi, On Tue, May 16, 2017 at 10:00:15PM +, Town, Brian A. (GSFC-428.0)[Embedded Flight Systems, Inc] wrote: > I was pointed to the spice-space page by someone after discussing > problems with VNC. I was under the impression spice only worked with > KVM, I am working with an ESXI based VM environment though and looking > for a promising VDI solution. Is it possible to use spice in this set > up? I'm not aware of a way to make spice work with vmware hypervisor. So the solution wouldn't be the same as we can do with KVM. If your guest is Linux on Xorg, you could try x11spice [0] and connect with a spice client like virt-viewer or remote-viewer (part of virt-viewer package) [1]. The remote-viewer client is also available in windows. [0] https://github.com/jwhite66/x11spice [1] https://virt-manager.org/download/ Cheers, toso signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
> On 17 May 2017, at 11:55, Christophe Fergeau wrote: > > On Wed, May 17, 2017 at 11:45:50AM +0200, Christophe de Dinechin wrote: >>> I'm really sure no architecture on hearth can do check and print in >>> a single machine instruction. >> >> This is not what I said. I said that the cost in the likely case was one >> extra instruction. >> >>> Actually I don't know any architecture >>> where a single check like this can be done in a single instruction. >> >> Reduced source code: https://pastebin.com/ibZEVThA. >> >> Machine code for x86 with clang: https://pastebin.com/Vmtwpkhf >> >> The test follows the call to malloc. It’s a single “testb” instruction >> followed by a never-taken conditional jump to cold-code. > > NB: feel free to annotate such tests with G_LIKELY/G_UNLIKELY Thanks. It’s already in my version, but I have not shared it yet. Christophe > > Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol v2] agent: Add support for reporting on free space
Hi Jakub,
On Tue, 2017-05-09 at 18:53 +0200, Jakub Janků wrote:
> Agent can send VDAgentFileXferStatusMessage with result
> VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE to indicate lack of free
> space. This enables more detailed error reporting, so the user knows
> why the file transfer has failed.
>
> Add VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS to ensure detailed errors
> are not sent to clients that do not support it. This can be used
> with more file xfer errors in the future.
> ---
> spice/vd_agent.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index 3b1f183..d2477ce 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -99,11 +99,16 @@ enum {
> VD_AGENT_FILE_XFER_STATUS_CANCELLED,
> VD_AGENT_FILE_XFER_STATUS_ERROR,
> VD_AGENT_FILE_XFER_STATUS_SUCCESS,
> +VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
> };
>
> typedef struct SPICE_ATTR_PACKED VDAgentFileXferStatusMessage {
> uint32_t id;
> uint32_t result;
> + /* Used to send additional data for detailed error messages
> +* to clients with VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS
> capability.
> +*/
the comment should be expanded to say the data type for the particular
status result. eg VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE uint64
> + uint8_t data[0];
> } VDAgentFileXferStatusMessage;
>
> typedef struct SPICE_ATTR_PACKED VDAgentFileXferStartMessage {
> @@ -248,6 +253,7 @@ enum {
> VD_AGENT_CAP_AUDIO_VOLUME_SYNC,
> VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
> VD_AGENT_CAP_FILE_XFER_DISABLED,
> +VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> VD_AGENT_END_CAP,
> };
>
besides that it looks good to me
Pavel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol v2] agent: Add support for reporting on free space
>
> Hi Jakub,
>
> On Tue, 2017-05-09 at 18:53 +0200, Jakub Janků wrote:
> > Agent can send VDAgentFileXferStatusMessage with result
> > VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE to indicate lack of free
> > space. This enables more detailed error reporting, so the user knows
> > why the file transfer has failed.
> >
> > Add VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS to ensure detailed errors
> > are not sent to clients that do not support it. This can be used
> > with more file xfer errors in the future.
> > ---
> > spice/vd_agent.h | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index 3b1f183..d2477ce 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -99,11 +99,16 @@ enum {
> > VD_AGENT_FILE_XFER_STATUS_CANCELLED,
> > VD_AGENT_FILE_XFER_STATUS_ERROR,
> > VD_AGENT_FILE_XFER_STATUS_SUCCESS,
> > +VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
> > };
> >
> > typedef struct SPICE_ATTR_PACKED VDAgentFileXferStatusMessage {
> > uint32_t id;
> > uint32_t result;
> > + /* Used to send additional data for detailed error messages
> > +* to clients with VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS
> > capability.
> > +*/
>
> the comment should be expanded to say the data type for the particular
> status result. eg VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE uint64
>
> > + uint8_t data[0];
> > } VDAgentFileXferStatusMessage;
> >
> > typedef struct SPICE_ATTR_PACKED VDAgentFileXferStartMessage {
> > @@ -248,6 +253,7 @@ enum {
> > VD_AGENT_CAP_AUDIO_VOLUME_SYNC,
> > VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
> > VD_AGENT_CAP_FILE_XFER_DISABLED,
> > +VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > VD_AGENT_END_CAP,
> > };
> >
Is this capability enough?
I means, this basically declare that the client wants any additional
details. What's happen in the future if another VD_AGENT_FILE_XFER_STATUS_xxx
is added? Should the agent just consider the additional constants as error and
ignore the attached data? What if in the future there's a status like SUCCESS
and CANCELLED which is not exactly an error? Will be handled as error.
Maybe we can use a some bits in the status (like bit 31) to mark proper
additional
error?
I assume an agent supporting VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS shold accept
additional data appended after VDAgentFileXferStatusMessage.
>
> besides that it looks good to me
>
> Pavel
>
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol v2] agent: Add support for reporting on free space
On Wed, 2017-05-17 at 07:17 -0400, Frediano Ziglio wrote:
> >
> > Hi Jakub,
> >
> > On Tue, 2017-05-09 at 18:53 +0200, Jakub Janků wrote:
> > > Agent can send VDAgentFileXferStatusMessage with result
> > > VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE to indicate lack of
> > > free
> > > space. This enables more detailed error reporting, so the user
> > > knows
> > > why the file transfer has failed.
> > >
> > > Add VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS to ensure detailed
> > > errors
> > > are not sent to clients that do not support it. This can be used
> > > with more file xfer errors in the future.
> > > ---
> > > spice/vd_agent.h | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > index 3b1f183..d2477ce 100644
> > > --- a/spice/vd_agent.h
> > > +++ b/spice/vd_agent.h
> > > @@ -99,11 +99,16 @@ enum {
> > > VD_AGENT_FILE_XFER_STATUS_CANCELLED,
> > > VD_AGENT_FILE_XFER_STATUS_ERROR,
> > > VD_AGENT_FILE_XFER_STATUS_SUCCESS,
> > > +VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
> > > };
> > >
> > > typedef struct SPICE_ATTR_PACKED VDAgentFileXferStatusMessage {
> > > uint32_t id;
> > > uint32_t result;
> > > + /* Used to send additional data for detailed error messages
> > > +* to clients with VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS
> > > capability.
> > > +*/
> >
> > the comment should be expanded to say the data type for the
> > particular
> > status result. eg VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE
> > uint64
> >
> > > + uint8_t data[0];
> > > } VDAgentFileXferStatusMessage;
> > >
> > > typedef struct SPICE_ATTR_PACKED VDAgentFileXferStartMessage {
> > > @@ -248,6 +253,7 @@ enum {
> > > VD_AGENT_CAP_AUDIO_VOLUME_SYNC,
> > > VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
> > > VD_AGENT_CAP_FILE_XFER_DISABLED,
> > > +VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > > VD_AGENT_END_CAP,
> > > };
> > >
>
> Is this capability enough?
> I means, this basically declare that the client wants any additional
> details. What's happen in the future if another
> VD_AGENT_FILE_XFER_STATUS_xxx
> is added? Should the agent just consider the additional constants as
> error and
> ignore the attached data? What if in the future there's a status
> like SUCCESS
> and CANCELLED which is not exactly an error? Will be handled as
> error.
it would have to be considered when implementing non error results,
the cap is for errors. The agent fallbacks to a general
VD_AGENT_FILE_XFER_STATUS_ERROR if the client does not have the cap.
> Maybe we can use a some bits in the status (like bit 31) to mark
> proper additional
> error?
imho instead of mixing result types and some flags is better to have a
new message type
> I assume an agent supporting VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS
> shold accept
> additional data appended after VDAgentFileXferStatusMessage.
In the current implementation the cap is only for the client. Client
may expect additional data (bigger message size) if it has the flag.
Thanks,
Pavel
>
> >
> > besides that it looks good to me
> >
> > Pavel
> >
>
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-vdagent v3] file-xfer: Check free space before file transfer
Hi,
On Wed, 2017-05-10 at 22:34 +0200, Jakub Janků wrote:
> Add function get_free_space_available that retrieves amount of free
> space in the given directory. The statvfs may fail even when there's
> enough free space (e.g. when not supported by system), in this case
> return G_MAXUINT64 so that the transfer isn't terminated
> groundlessly.
>
> When the file is too big, send VDAgentFileXferStatusMessage with
> result VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE and amount of free
> space available, if the result isn't supported by client, send
> VD_AGENT_FILE_XFER_STATUS_ERROR. Client then terminates the
> transfer.
>
> Add send_file_xfer_status_detailed() which can send file xfer status
> messages with additional (error) data, could be used for reporting
> more file xfer errors.
> ---
> src/vdagent/file-xfers.c | 34
> src/vdagentd/vdagentd.c | 68 +++
> -
> 2 files changed, 84 insertions(+), 18 deletions(-)
>
> diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
> index b3937a4..3ed139a 100644
> --- a/src/vdagent/file-xfers.c
> +++ b/src/vdagent/file-xfers.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -168,6 +169,17 @@ error:
> return NULL;
> }
>
> +static uint64_t get_free_space_available(const char *path)
> +{
> +struct statvfs stat;
> +if (statvfs(path, &stat) != 0) {
> +syslog(LOG_WARNING, "file-xfer: failed to get free space,
> statvfs error: %s",
> + strerror(errno));
> +return G_MAXUINT64;
> +}
> +return stat.f_bsize * stat.f_bavail;
> +}
> +
> void vdagent_file_xfers_start(struct vdagent_file_xfers *xfers,
> VDAgentFileXferStartMessage *msg)
> {
> @@ -175,6 +187,7 @@ void vdagent_file_xfers_start(struct
> vdagent_file_xfers *xfers,
> char *dir = NULL, *path = NULL, *file_path = NULL;
> struct stat st;
> int i;
> +uint64_t free_space;
>
> g_return_if_fail(xfers != NULL);
>
> @@ -193,6 +206,27 @@ void vdagent_file_xfers_start(struct
> vdagent_file_xfers *xfers,
>
> file_path = g_build_filename(xfers->save_dir, task->file_name,
> NULL);
>
> +free_space = get_free_space_available(xfers->save_dir);
> +if (task->file_size > free_space) {
> +gchar *free_space_str = g_format_size(free_space);
g_format_size is available since GLIB 2.30, however vdagent requires
2.28. You can simplify the error or use the GLIB_CHECK_VERSION macro
to compile the code conditionally.
> +gchar *file_size_str = g_format_size(task->file_size);
> +syslog(LOG_ERR, "file-xfer: not enough free space (%s to
> copy, %s free)",
> + file_size_str, free_space_str);
> +g_free(free_space_str);
> +g_free(file_size_str);
> +
> +udscs_write(xfers->vdagentd,
> +VDAGENTD_FILE_XFER_STATUS,
> +msg->id,
> +VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
> +(uint8_t *)&free_space,
> +sizeof(free_space));
> +vdagent_file_xfer_task_free(task);
> +g_free(file_path);
> +g_free(dir);
> +return;
> +}
> +
> dir = g_path_get_dirname(file_path);
> if (g_mkdir_with_parents(dir, S_IRWXU) == -1) {
> syslog(LOG_ERR, "file-xfer: Failed to create dir %s", dir);
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index f3ac606..39fdb04 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -301,20 +301,45 @@ static void do_client_clipboard(struct
> vdagent_virtio_port *vport,
> data, size);
> }
>
> +static void send_file_xfer_status_detailed(struct
> vdagent_virtio_port *vport,
> + const char *msg,
> + uint32_t id, uint32_t
> xfer_status,
> + uint8_t *data, uint32_t
> data_size)
> +{
> +VDAgentFileXferStatusMessage *status;
> +
> +/* Replace new detailed errors with older generic
> VD_AGENT_FILE_XFER_STATUS_ERROR
> + * when not supported by client */
> +if (xfer_status > VD_AGENT_FILE_XFER_STATUS_SUCCESS &&
> +!VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> +VD_AGENT_CAP_FILE_XFER_DETAILED_ERR
> ORS)) {
> +xfer_status = VD_AGENT_FILE_XFER_STATUS_ERROR;
> +data_size = 0;
> +}
> +
> +status = malloc(sizeof(*status) + data_size);
> +status->id = GUINT32_TO_LE(id);
> +status->result = GUINT32_TO_LE(xfer_status);
> +if (data)
> +memcpy(status->data, data, data_size);
> +
> +if (msg)
> +syslog(LOG_WARNING, msg, id);
I see you copied it, but in general it requires msg to always have
"%u". Imho is better to always prefix the msg with "file-xfer %u:". Or
to support format strings properly (overkill im
[Spice-devel] [PATCH] spicy: add gstreamer options to command line
From: Victor Toso
So we can see all available options with spicy --help-gst and set them
as command line argument.
Signed-off-by: Victor Toso
---
tools/Makefile.am | 4
tools/spicy.c | 9 +
2 files changed, 13 insertions(+)
diff --git a/tools/Makefile.am b/tools/Makefile.am
index c80d34a..1e3deed 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -29,11 +29,15 @@ spicy_LDADD = \
$(top_builddir)/src/libspice-client-gtk-3.0.la \
$(top_builddir)/src/libspice-client-glib-2.0.la \
$(GTK_LIBS) \
+ $(GSTAUDIO_LIBS) \
+ $(GSTVIDEO_LIBS) \
$(NULL)
# FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
spicy_CPPFLAGS = \
$(TOOLS_CPPFLAGS) \
+ $(GSTAUDIO_CFLAGS) \
+ $(GSTVIDEO_CFLAGS) \
-DSPICE_DISABLE_DEPRECATED \
-Wno-deprecated-declarations\
$(NULL)
diff --git a/tools/spicy.c b/tools/spicy.c
index eeb640d..40cd6b3 100644
--- a/tools/spicy.c
+++ b/tools/spicy.c
@@ -37,6 +37,9 @@
#include "usb-device-widget.h"
#include "spicy-connect.h"
+#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
+#include
+#endif
typedef struct spice_connection spice_connection;
@@ -1995,6 +1998,9 @@ int main(int argc, char *argv[])
/* parse opts */
gtk_init(&argc, &argv);
+#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
+gst_init(&argc, &argv);
+#endif
context = g_option_context_new("- spice client test application");
g_option_context_set_summary(context, "Gtk+ test client to connect to
Spice servers.");
g_option_context_set_description(context, "Report bugs to "
PACKAGE_BUGREPORT ".");
@@ -2002,6 +2008,9 @@ int main(int argc, char *argv[])
g_option_context_set_main_group(context, spice_cmdline_get_option_group());
g_option_context_add_main_entries(context, cmd_entries, NULL);
g_option_context_add_group(context, gtk_get_option_group(TRUE));
+#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
+g_option_context_add_group(context, gst_init_get_option_group());
+#endif
if (!g_option_context_parse (context, &argc, &argv, &error)) {
g_print("option parsing failed: %s\n", error->message);
exit(1);
--
2.13.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] spicy: add gstreamer options to command line
On Wed, 2017-05-17 at 13:51 +0200, Victor Toso wrote:
> From: Victor Toso
>
> So we can see all available options with spicy --help-gst and set
> them
> as command line argument.
Do you have an usage example?
Thanks,
Pavel
>
> Signed-off-by: Victor Toso
> ---
> tools/Makefile.am | 4
> tools/spicy.c | 9 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index c80d34a..1e3deed 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -29,11 +29,15 @@ spicy_LDADD = \
> $(top_builddir)/src/libspice-client-gtk-3.0.la \
> $(top_builddir)/src/libspice-client-glib-2.0.la \
> $(GTK_LIBS) \
> + $(GSTAUDIO_LIBS) \
> + $(GSTVIDEO_LIBS) \
> $(NULL)
>
> # FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
> spicy_CPPFLAGS = \
> $(TOOLS_CPPFLAGS) \
> + $(GSTAUDIO_CFLAGS) \
> + $(GSTVIDEO_CFLAGS) \
> -DSPICE_DISABLE_DEPRECATED \
> -Wno-deprecated-declarations\
> $(NULL)
> diff --git a/tools/spicy.c b/tools/spicy.c
> index eeb640d..40cd6b3 100644
> --- a/tools/spicy.c
> +++ b/tools/spicy.c
> @@ -37,6 +37,9 @@
> #include "usb-device-widget.h"
>
> #include "spicy-connect.h"
> +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> +#include
> +#endif
>
> typedef struct spice_connection spice_connection;
>
> @@ -1995,6 +1998,9 @@ int main(int argc, char *argv[])
>
> /* parse opts */
> gtk_init(&argc, &argv);
> +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> +gst_init(&argc, &argv);
> +#endif
> context = g_option_context_new("- spice client test
> application");
> g_option_context_set_summary(context, "Gtk+ test client to
> connect to Spice servers.");
> g_option_context_set_description(context, "Report bugs to "
> PACKAGE_BUGREPORT ".");
> @@ -2002,6 +2008,9 @@ int main(int argc, char *argv[])
> g_option_context_set_main_group(context,
> spice_cmdline_get_option_group());
> g_option_context_add_main_entries(context, cmd_entries, NULL);
> g_option_context_add_group(context,
> gtk_get_option_group(TRUE));
> +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> +g_option_context_add_group(context,
> gst_init_get_option_group());
> +#endif
> if (!g_option_context_parse (context, &argc, &argv, &error)) {
> g_print("option parsing failed: %s\n", error->message);
> exit(1);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] spicy: add gstreamer options to command line
Hi,
On Wed, May 17, 2017 at 01:59:44PM +0200, Pavel Grunt wrote:
> On Wed, 2017-05-17 at 13:51 +0200, Victor Toso wrote:
> > From: Victor Toso
> >
> > So we can see all available options with spicy --help-gst and set
> > them
> > as command line argument.
>
> Do you have an usage example?
Sure, for me the most interesting ones are --gst-debug-level=4 (which
helps me to know which elements are being used in our pipelines) and
often I do --gst-plugin-path=/usr/lib64/gstreamer-1.0 to load plugins
from my system instead of JHBuild environment.
Both of them are available using environment variables but command line
support is easier to set (even more in windows)
All options in --help-gst are:
--gst-versionPrint the GStreamer version
--gst-fatal-warnings Make all warnings fatal
--gst-debug-help Print available debug categories and exit
--gst-debug-level=LEVEL Default debug level from 1 (only error) to 9
(anything) or 0 for no output
--gst-debug=LIST Comma-separated list of category_name:level
pairs to set specific levels for the individual categories. Example:
GST_AUTOPLUG:5,GST_ELEMENT_*:3
--gst-debug-no-color Disable colored debugging output
--gst-debug-color-mode Changes coloring mode of the debug log.
Possible modes: off, on, disable, auto, unix
--gst-debug-disable Disable debugging
--gst-plugin-spewEnable verbose plugin loading diagnostics
--gst-plugin-path=PATHS Colon-separated paths containing plugins
--gst-plugin-load=PLUGINSComma-separated list of plugins to preload in
addition to the list stored in environment variable GST_PLUGIN_PATH
--gst-disable-segtrapDisable trapping of segmentation faults
during plugin loading
--gst-disable-registry-updateDisable updating the registry
--gst-disable-registry-fork Disable spawning a helper process while
scanning the registry
Cheers,
>
> Thanks,
> Pavel
>
> >
> > Signed-off-by: Victor Toso
> > ---
> > tools/Makefile.am | 4
> > tools/spicy.c | 9 +
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index c80d34a..1e3deed 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -29,11 +29,15 @@ spicy_LDADD = \
> > $(top_builddir)/src/libspice-client-gtk-3.0.la \
> > $(top_builddir)/src/libspice-client-glib-2.0.la \
> > $(GTK_LIBS) \
> > + $(GSTAUDIO_LIBS) \
> > + $(GSTVIDEO_LIBS) \
> > $(NULL)
> >
> > # FIXME: GtkAction and lots of GtkUIManager APIs are deprecated
> > spicy_CPPFLAGS = \
> > $(TOOLS_CPPFLAGS) \
> > + $(GSTAUDIO_CFLAGS) \
> > + $(GSTVIDEO_CFLAGS) \
> > -DSPICE_DISABLE_DEPRECATED \
> > -Wno-deprecated-declarations\
> > $(NULL)
> > diff --git a/tools/spicy.c b/tools/spicy.c
> > index eeb640d..40cd6b3 100644
> > --- a/tools/spicy.c
> > +++ b/tools/spicy.c
> > @@ -37,6 +37,9 @@
> > #include "usb-device-widget.h"
> >
> > #include "spicy-connect.h"
> > +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> > +#include
> > +#endif
> >
> > typedef struct spice_connection spice_connection;
> >
> > @@ -1995,6 +1998,9 @@ int main(int argc, char *argv[])
> >
> > /* parse opts */
> > gtk_init(&argc, &argv);
> > +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> > +gst_init(&argc, &argv);
> > +#endif
> > context = g_option_context_new("- spice client test
> > application");
> > g_option_context_set_summary(context, "Gtk+ test client to
> > connect to Spice servers.");
> > g_option_context_set_description(context, "Report bugs to "
> > PACKAGE_BUGREPORT ".");
> > @@ -2002,6 +2008,9 @@ int main(int argc, char *argv[])
> > g_option_context_set_main_group(context,
> > spice_cmdline_get_option_group());
> > g_option_context_add_main_entries(context, cmd_entries, NULL);
> > g_option_context_add_group(context,
> > gtk_get_option_group(TRUE));
> > +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> > +g_option_context_add_group(context,
> > gst_init_get_option_group());
> > +#endif
> > if (!g_option_context_parse (context, &argc, &argv, &error)) {
> > g_print("option parsing failed: %s\n", error->message);
> > exit(1);
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Using spice in an ESXI environment
Thanks toso, downloaded it and attempted to build it out but getting an error during make. Autogen and configure succeeded without issue. In function `check_binary': /home/btown/x11spice-master/src/tests/tests.c:83: undefined reference to `g_log' /home/btown/x11spice-master/src/tests/tests.c:84: undefined reference to `g_test_skip' tests.o: In function `test_common_start': /home/btown/x11spice-master/src/tests/tests.c:46: undefined reference to `g_test_build_filename' /home/btown/x11spice-master/src/tests/tests.c:56: undefined reference to `g_log' /home/btown/x11spice-master/src/tests/tests.c:57: undefined reference to `g_test_fail' /home/btown/x11spice-master/src/tests/tests.c:39: undefined reference to `g_test_skip' /home/btown/x11spice-master/src/tests/tests.c:48: undefined reference to `g_log' /home/btown/x11spice-master/src/tests/tests.c:49: undefined reference to `g_test_fail' tests.o: In function `check_screenshot': /home/btown/x11spice-master/src/tests/tests.c:102: undefined reference to `g_test_build_filename' /home/btown/x11spice-master/src/tests/tests.c:119: undefined reference to `g_log' /home/btown/x11spice-master/src/tests/tests.c:120: undefined reference to `g_log' /home/btown/x11spice-master/src/tests/tests.c:121: undefined reference to `g_test_fail' /home/btown/x11spice-master/src/tests/tests.c:123: undefined reference to `g_free' There are a ton more undefined reference errors as well before it just stalls out collect2: error: ld returned 1 exit status Makefile:538: recipe for target 'x11spice_test' failed make[2]: *** [x11spice_test] Error 1 make[2]: Leaving directory '/home/btown/x11spice-master/src/tests' Makefile:669: recipe for target 'check-recursive' failed make[1]: *** [check-recursive] Error 1 make[1]: Leaving directory '/home/btown/x11spice-master/src' Makefile:361: recipe for target 'check-recursive' failed make: *** [check-recursive] Error 1 Attempting to build it on an Ubuntu 16.04 VM. Package: xserver-xorg Priority: optional Section: x11 Installed-Size: 240 Maintainer: Ubuntu X-SWAT Original-Maintainer: Debian X Strike Force Architecture: amd64 Source: xorg Version: 1:7.7+13ubuntu3 Provides: xserver Depends: xserver-xorg-core (>= 2:1.17.2-2), xserver-xorg-input-all | xorg-driver-input, xkb-data (>= 1.4), x11-xkb-utils Recommends: libgl1-mesa-dri, xserver-xorg-video-all | xorg-driver-video Filename: pool/main/x/xorg/xserver-xorg_7.7+13ubuntu3_amd64.deb Size: 57178 MD5sum: 2179d2c5f2de9f82f413f2ce68a7aaea SHA1: 4461ef43eed3e1a05ba4fa87b8ce22b7fe6a7de2 SHA256: 67435708861b8fa5a8dfc97e90c778ed9d4b9a33da37ffb662c53d58004c4879 Description-en: X.Org X server This package depends on the full suite of the server and drivers for the X.Org X server. It does not provide the actual server itself. Description-md5: 3d8c1d268e8af6b69f54d86fbd5a3939 Homepage: http://www.x.org/ Brian Town Landsat 8 Goddard Space Flight Center -Original Message- From: Victor Toso [mailto:victort...@redhat.com] Sent: Wednesday, May 17, 2017 6:14 AM To: Town, Brian A. (GSFC-428.0)[Embedded Flight Systems, Inc] Cc: spice-devel@lists.freedesktop.org Subject: Re: [Spice-devel] Using spice in an ESXI environment Hi, On Tue, May 16, 2017 at 10:00:15PM +, Town, Brian A. (GSFC-428.0)[Embedded Flight Systems, Inc] wrote: > I was pointed to the spice-space page by someone after discussing > problems with VNC. I was under the impression spice only worked with > KVM, I am working with an ESXI based VM environment though and looking > for a promising VDI solution. Is it possible to use spice in this set > up? I'm not aware of a way to make spice work with vmware hypervisor. So the solution wouldn't be the same as we can do with KVM. If your guest is Linux on Xorg, you could try x11spice [0] and connect with a spice client like virt-viewer or remote-viewer (part of virt-viewer package) [1]. The remote-viewer client is also available in windows. [0] https://github.com/jwhite66/x11spice [1] https://virt-manager.org/download/ Cheers, toso ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/5] Avoid clang warnings on casts with stricter alignment requirements
> On 17 May 2017, at 12:08, Frediano Ziglio wrote:
>
>>
>>
>>> On 17 May 2017, at 09:44, Frediano Ziglio wrote:
>>>
From: Christophe de Dinechin
For example, something like this:
uint8_t *p8;
uint32_t *p32 = (uint32_t *) p8;
generates a warning like this:
spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
*') to
'uint32_t *' (aka 'unsigned int *') increases required alignment from
1
to
4 [-Werror,-Wcast-align]
The warning indicates that we end up with a pointer to data that
should be 4-byte aligned, but its value may be misaligned. On x86,
this does not make much of a difference, except a relatively minor
performance penalty. However, on platforms such as ARM, misaligned
accesses are emulated by the kernel, and support for them is optional.
So we may end up with a fault.
The intent of the fix here is to make it easy to identify and rework
places where actual mis-alignment occurs. Wherever casts raise the
warning,
they are replaced with a macro:
- SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
we believe the resulting pointer is aligned. If it is not, a warning will
be issued.
- SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates
that
we believe the resulting pointer is not always aligned
Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
to improve performance, e.g. by using memcpy.
There are normally no warnings for SPICE_UNALIGNED_CAST, but it is
possible
to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
Signed-off-by: Christophe de Dinechin
---
spice-common| 2 +-
src/channel-cursor.c| 6 +++---
src/channel-display-mjpeg.c | 2 +-
src/decode-glz-tmpl.c | 2 +-
src/spice-channel.c | 14 +-
5 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/spice-common b/spice-common
index af682b1..1239c82 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
+Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
diff --git a/src/channel-cursor.c b/src/channel-cursor.c
index 609243b..50de5ce 100644
--- a/src/channel-cursor.c
+++ b/src/channel-cursor.c
@@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel
*channel,
SpiceCursor *scursor)
memcpy(cursor->data, data, size);
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-if (pix_mask && *((guint32*)data + i) == 0xff) {
+if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
cursor->data[i] |= (pix_mask ? 0 : 0xff00);
@@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel
*channel,
SpiceCursor *scursor)
case SPICE_CURSOR_TYPE_COLOR16:
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-pix = *((guint16*)data + i);
+pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
if (pix_mask && pix == 0x7fff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
>>>
>>> If these 2 hits you are going to have a message for every single pixel
>>> in the image, potentially millions.
>>
>> No, you will get one message only when you enter the image.
>>
>>static const char *last_loc = NULL;
>>if (loc != last_loc) {
>>last_loc = loc;
>>spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_WARNING, loc,
>>__FUNCTION__,
>> "Misaligned access at %p, alignment %u", p, sz);
>>}
>>
>> If it’s the same source code location, you get it once. But if you have two
>> such warnings, then yes, it may become verbose.
>>
>> But if you get millions of calls in case of mis-alignment, it means that on
>> ARM, you get millions of faults into the kernel to correct the alignment,
>> which is surely a lot worse performance-wise than a call.
>>
>> I remember someone (Pavel?) recently saying that Spice performance on ARM was
>> terrible. Something like this could easily explain it.
>>
>>
>>> I'm still convinced, like in this case, that if we are sure that data
>>> should be aligned no check and warning should be done.
>>
>> Would you be OK with a check and warning under #ifndef NDEBUG (like an
>> assert).
>>
>>> I'm really sure no architecture on hearth can do
[Spice-devel] [PATCH v2 0/2] macOS enablement patches
From: Christophe de Dinechin This patch set eliminates warnings detected by clang with respect to type alignments. Vittorio Toso had submitted something similar. In this version, I took into account comments by Christophe Fergeau regarding how to know which casts were aligned and which ones were not aligned. I added some instrumentation to detect the two cases. v2 changes: - Fix alignment used for checking (alignment of pointed type, not pointer) - Disable checking entirely if NDEBUG is defined. - Add G_UNLIKELY to indicate that the branch is not likely to be taken I saw no obvious equivalent of NDEBUG in Spice code, so I used NDEBUG, which is the macro used to disable assert() defined in . Christophe de Dinechin (2): Style adjustment Avoid clang warnings on casts with stricter alignment requirements common/canvas_base.c | 14 --- common/mem.c | 24 common/mem.h | 39 ++- common/sw_canvas.c | 104 +-- 4 files changed, 120 insertions(+), 61 deletions(-) -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 2/2] Avoid clang warnings on casts with stricter alignment requirements
From: Christophe de Dinechin
For example, something like this:
uint8_t *p8;
uint32_t *p32 = (uint32_t *) p8;
generates a warning like this:
spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char *')
to
'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to
4 [-Werror,-Wcast-align]
The warning indicates that we end up with a pointer to data that
should be 4-byte aligned, but its value may be misaligned. On x86,
this does not make much of a difference, except a relatively minor
performance penalty. However, on platforms such as ARM, misaligned
accesses are emulated by the kernel, and support for them is optional.
So we may end up with a fault.
The intent of the fix here is to make it easy to identify and rework
places where actual mis-alignment occurs. Wherever casts raise the warning,
they are replaced with a macro:
- SPICE_ALIGNED_CAST(type, value) casts value to type, and indicates that
we believe the resulting pointer is aligned. If it is not, a warning will
be issued.
- SPICE_UNALIGNED_CAST(type, value) casts value to type, and indicates that
we believe the resulting pointer is not always aligned
Any code using SPICE_UNALIGNED_CAST may need to be revisited in order
to improve performance, e.g. by using memcpy.
There are normally no warnings for SPICE_UNALIGNED_CAST, but it is possible
to emit debug messages for mis-alignment in SPICE_UNALIGNED_CAST.
Set environment variable SPICE_DEBUG_ALIGNMENT to activate this check.
All run-time checks are disabled when the NDEBUG macro is set
(like for the assert() macro from ).
Signed-off-by: Christophe de Dinechin
---
common/canvas_base.c | 14 +-
common/mem.c | 24
common/mem.h | 39 +--
common/sw_canvas.c | 10 ++
4 files changed, 76 insertions(+), 11 deletions(-)
diff --git a/common/canvas_base.c b/common/canvas_base.c
index 8f653e0..5815e9c 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -389,7 +389,7 @@ static pixman_image_t *canvas_get_quic(CanvasBase *canvas,
SpiceImage *image,
quic_data->current_chunk = 0;
if (quic_decode_begin(quic_data->quic,
- (uint32_t *)image->u.quic.data->chunk[0].data,
+ SPICE_UNALIGNED_CAST(uint32_t
*,image->u.quic.data->chunk[0].data),
image->u.quic.data->chunk[0].len >> 2,
&type, &width, &height) == QUIC_ERROR) {
spice_warning("quic decode begin failed");
@@ -523,8 +523,8 @@ static void canvas_fix_alignment(uint8_t *bits,
uint8_t *dest = bits;
for (row = height - 1; row > 0; --row) {
uint32_t *dest_aligned, *dest_misaligned;
-dest_aligned = (uint32_t *)(dest + stride_pixman*row);
-dest_misaligned = (uint32_t*)(dest + stride_encoded*row);
+dest_aligned = SPICE_ALIGNED_CAST(uint32_t *,dest +
stride_pixman*row);
+dest_misaligned = SPICE_UNALIGNED_CAST(uint32_t*,dest +
stride_encoded*row);
memmove(dest_aligned, dest_misaligned, stride_encoded);
}
}
@@ -1889,7 +1889,8 @@ static int quic_usr_more_space(QuicUsrContext *usr,
uint32_t **io_ptr, int rows_
}
quic_data->current_chunk++;
-*io_ptr = (uint32_t
*)quic_data->chunks->chunk[quic_data->current_chunk].data;
+*io_ptr = SPICE_ALIGNED_CAST(uint32_t *,
+
quic_data->chunks->chunk[quic_data->current_chunk].data);
return quic_data->chunks->chunk[quic_data->current_chunk].len >> 2;
}
@@ -1945,6 +1946,7 @@ static void canvas_mask_pixman(CanvasBase *canvas,
int needs_invert;
pixman_region32_t mask_region;
uint32_t *mask_data;
+uint8_t *mask_data_src;
int mask_x, mask_y;
int mask_width, mask_height, mask_stride;
pixman_box32_t extents;
@@ -2006,7 +2008,9 @@ static void canvas_mask_pixman(CanvasBase *canvas,
/* round down X to even 32 pixels (i.e. uint32_t) */
extents.x1 = extents.x1 & ~(0x1f);
-mask_data = (uint32_t *)((uint8_t *)mask_data + mask_stride * extents.y1 +
extents.x1 / 32);
+mask_data_src = (uint8_t *)mask_data + mask_stride * extents.y1 +
extents.x1 / 32;
+mask_data = SPICE_UNALIGNED_CAST(uint32_t *, mask_data_src);
+
mask_x -= extents.x1;
mask_y -= extents.y1;
mask_width = extents.x2 - extents.x1;
diff --git a/common/mem.c b/common/mem.c
index e430b5d..5b15bb6 100644
--- a/common/mem.c
+++ b/common/mem.c
@@ -294,3 +294,27 @@ size_t spice_buffer_remove(SpiceBuffer *buffer, size_t len)
buffer->offset -= len;
return len;
}
+
+void spice_alignment_warning(const char *loc, void *p, unsigned sz)
+{
+static const char *last_loc = NULL;
+if (loc != last_loc) {
+last_loc = loc;
+spice_log(SPICE_LOG_DOMAIN, SPICE_LOG_LEVEL_WARNING, loc, __FUNCTION__,
+ "Misaligned access at %p, a
[Spice-devel] [PATCH v2 1/2] Style adjustment
From: Christophe de Dinechin
Fixing a piece of code that does not match the style for the
rest of the source files
Signed-off-by: Christophe de Dinechin
---
common/sw_canvas.c | 96 +-
1 file changed, 45 insertions(+), 51 deletions(-)
diff --git a/common/sw_canvas.c b/common/sw_canvas.c
index 959421f..531d608 100644
--- a/common/sw_canvas.c
+++ b/common/sw_canvas.c
@@ -1189,16 +1189,15 @@ static void canvas_destroy(SpiceCanvas *spice_canvas)
static SpiceCanvasOps sw_canvas_ops;
static SpiceCanvas *canvas_create_common(pixman_image_t *image,
- uint32_t format
- , SpiceImageCache *bits_cache
+ uint32_t format,
+ SpiceImageCache *bits_cache,
#ifdef SW_CANVAS_CACHE
- , SpicePaletteCache *palette_cache
+ SpicePaletteCache *palette_cache,
#endif
- , SpiceImageSurfaces *surfaces
- , SpiceGlzDecoder *glz_decoder
- , SpiceJpegDecoder *jpeg_decoder
- , SpiceZlibDecoder *zlib_decoder
- )
+ SpiceImageSurfaces *surfaces,
+ SpiceGlzDecoder *glz_decoder,
+ SpiceJpegDecoder *jpeg_decoder,
+ SpiceZlibDecoder *zlib_decoder)
{
SwCanvas *canvas;
@@ -1207,18 +1206,17 @@ static SpiceCanvas *canvas_create_common(pixman_image_t
*image,
canvas = spice_new0(SwCanvas, 1);
canvas_base_init(&canvas->base, &sw_canvas_ops,
- pixman_image_get_width (image),
- pixman_image_get_height (image),
- format
- , bits_cache
+ pixman_image_get_width (image),
+ pixman_image_get_height (image),
+ format,
+ bits_cache,
#ifdef SW_CANVAS_CACHE
- , palette_cache
+ palette_cache,
#endif
- , surfaces
- , glz_decoder
- , jpeg_decoder
- , zlib_decoder
- );
+ surfaces,
+ glz_decoder,
+ jpeg_decoder,
+ zlib_decoder);
canvas->private_data = NULL;
canvas->private_data_size = 0;
@@ -1227,61 +1225,57 @@ static SpiceCanvas *canvas_create_common(pixman_image_t
*image,
return (SpiceCanvas *)canvas;
}
-SpiceCanvas *canvas_create(int width, int height, uint32_t format
- , SpiceImageCache *bits_cache
+SpiceCanvas *canvas_create(int width, int height, uint32_t format,
+ SpiceImageCache *bits_cache,
#ifdef SW_CANVAS_CACHE
- , SpicePaletteCache *palette_cache
+ SpicePaletteCache *palette_cache,
#endif
- , SpiceImageSurfaces *surfaces
- , SpiceGlzDecoder *glz_decoder
- , SpiceJpegDecoder *jpeg_decoder
- , SpiceZlibDecoder *zlib_decoder
- )
+ SpiceImageSurfaces *surfaces,
+ SpiceGlzDecoder *glz_decoder,
+ SpiceJpegDecoder *jpeg_decoder,
+ SpiceZlibDecoder *zlib_decoder)
{
pixman_image_t *image;
image = pixman_image_create_bits(spice_surface_format_to_pixman (format),
width, height, NULL, 0);
-return canvas_create_common(image, format
-, bits_cache
+return canvas_create_common(image, format,
+bits_cache,
#ifdef SW_CANVAS_CACHE
-, palette_cache
+palette_cache,
#endif
-, surfaces
-, glz_decoder
-, jpeg_decoder
-, zlib_decoder
-);
+surfaces,
+glz_decoder,
+jpeg_decoder,
+zlib_decoder);
}
SpiceCanvas *canvas_create_for_data(int width, int height, uint32_t format,
-uint8_t *data, int stride
- , SpiceImageCache *bits_cache
+uint8_t *data, int stride,
+SpiceImageCache *bits_
[Spice-devel] [PATCH vd_agent_win] Use sprintf_s instead of sprintf to not crash
---
vdagent/file_xfer.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
index de1aea1..de98d50 100644
--- a/vdagent/file_xfer.cpp
+++ b/vdagent/file_xfer.cpp
@@ -113,8 +113,8 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage*
start,
if (attempt == 0) {
strcpy(dest_filename, file_name);
} else {
-sprintf(dest_filename, "%.*s (%d)%s", int(extension - file_name),
file_name,
-attempt, extension);
+sprintf_s(dest_filename, SPICE_N_ELEMENTS(file_name) + POSTFIX_LEN,
+ "%.*s (%d)%s", int(extension - file_name), file_name,
attempt, extension);
}
if ((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1, file_path +
wlen, MAX_PATH - wlen)) == 0) {
vd_printf("failed converting file_name:%s to WideChar",
dest_filename);
--
2.13.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH vd_agent_win] Use sprintf_s instead of sprintf to not crash
>
> ---
> vdagent/file_xfer.cpp | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index de1aea1..de98d50 100644
> --- a/vdagent/file_xfer.cpp
> +++ b/vdagent/file_xfer.cpp
> @@ -113,8 +113,8 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage*
> start,
> if (attempt == 0) {
> strcpy(dest_filename, file_name);
> } else {
> -sprintf(dest_filename, "%.*s (%d)%s", int(extension -
> file_name), file_name,
> -attempt, extension);
> +sprintf_s(dest_filename, SPICE_N_ELEMENTS(file_name) +
> POSTFIX_LEN,
> + "%.*s (%d)%s", int(extension - file_name), file_name,
> attempt, extension);
> }
> if ((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1, file_path +
> wlen, MAX_PATH - wlen)) == 0) {
> vd_printf("failed converting file_name:%s to WideChar",
> dest_filename);
As far as I know dest_filename is allocate big enough to avoid the
overflow.
Did it crash for you or just a warning from the compiler?
This would be easier
sprintf_s(dest_filename, SPICE_N_ELEMENTS(dest_filename),
"%.*s (%d)%s", int(extension - file_name), file_name,
attempt, extension);
without computing again the length.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 4/4] vdagent: Use glib's commandline parser
On Tue, May 16, 2017 at 07:27:55PM +0200, Victor Toso wrote:
> From: Victor Toso
>
> As we already depend on glib, let's remove code that glib can take
> care. In this case, we don't need to handle commandline parsing
> ourselves.
>
> In regard the global variables:
>
> * static const char * -> static char * [only paths]
> path variables: portdev, fx_dir, vdagentd_socket
>
> * static int -> static gboolean
> flags: debug, do_daemonize, x11_sync, fx_open_dir
>
> Signed-off-by: Victor Toso
> ---
> src/vdagent/vdagent.c | 117
> ++
> 1 file changed, 60 insertions(+), 57 deletions(-)
>
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 3d07d10..8399d5a 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -45,17 +45,45 @@
> #include "x11.h"
> #include "file-xfers.h"
>
> -static const char *portdev = DEFAULT_VIRTIO_PORT_PATH;
> -static const char *vdagentd_socket = VDAGENTD_SOCKET;
> -static int debug = 0;
> -static const char *fx_dir = NULL;
> -static int fx_open_dir = -1;
> static struct vdagent_x11 *x11 = NULL;
> static struct vdagent_file_xfers *vdagent_file_xfers = NULL;
> static struct udscs_connection *client = NULL;
> static int quit = 0;
> static int version_mismatch = 0;
>
> +/* Command line options */
> +static gboolean debug = FALSE;
> +static gboolean x11_sync = FALSE;
> +static gboolean fx_open_dir = FALSE;
> +static gboolean do_daemonize = TRUE;
> +static char *fx_dir = NULL;
> +static char *portdev = NULL;
> +static char *vdagentd_socket = NULL;
> +
> +static GOptionEntry entries[] ={
> +{ "debug", 'd', 0,
> + G_OPTION_ARG_NONE, &debug,
> + "Enable debug", NULL },
> +{ "virtio-serial-port-path", 's', 0,
> + G_OPTION_ARG_STRING, &portdev,
> + "Set virtio-serial path (" DEFAULT_VIRTIO_PORT_PATH ")", NULL },
> +{ "vdagentd-socket", 'S', 0, G_OPTION_ARG_STRING,
> + &vdagentd_socket,
> + "Set spice-vdagentd socket (" VDAGENTD_SOCKET ")", NULL },
> +{ "foreground", 'x', G_OPTION_FLAG_REVERSE,
> + G_OPTION_ARG_NONE, &do_daemonize,
> + "Do not daemonize the agent", NULL },
> +{ "file-xfer-save-dir", 'f', 0,
> + G_OPTION_ARG_STRING, &fx_dir,
> + "Set directory to file transfers files", NULL},
> +{ "file-xfer-open-dir", 'o', 0,
> + G_OPTION_ARG_NONE, &fx_open_dir,
> + "Open directory after completing file transfer", NULL },
> +{ "x11-abort-on-error", 'y', 0,
> + G_OPTION_ARG_NONE, &x11_sync,
> + "Aborts on errors from X11", NULL },
> +};
> +
> /**
> * xfer_get_download_directory
> *
> @@ -213,22 +241,6 @@ static int client_setup(int reconnect)
> return client == NULL;
> }
>
> -static void usage(FILE *fp)
> -{
> -fprintf(fp,
> - "Usage: spice-vdagent [OPTIONS]\n\n"
> - "Spice guest agent X11 session agent, version %s.\n\n"
> - "Options:\n"
> - " -hprint this text\n"
> - " -dlog debug messages\n"
> - " -s set virtio serial port\n"
> - " -S set udcs socket\n"
> - " -xdon't daemonize\n"
> - " -f file xfer save dir\n"
> - " -o <0|1> open dir on file xfer
> completion\n",
> - VERSION);
> -}
> -
> static void quit_handler(int sig)
> {
> quit = 1;
> @@ -292,45 +304,33 @@ static int file_test(const char *path)
> int main(int argc, char *argv[])
> {
> fd_set readfds, writefds;
> -int c, n, nfds, x11_fd;
> -int do_daemonize = 1;
> +int n, nfds, x11_fd;
> int parent_socket = 0;
> -int x11_sync = 0;
> struct sigaction act;
> +GOptionContext *context;
> +GError *error = NULL;
> +
> +context = g_option_context_new(NULL);
> +g_option_context_add_main_entries(context, entries, NULL);
> +g_option_context_set_summary(context,
> + "\tSpice session guest agent: X11\n"
> + "\tVersion: " VERSION);
> +g_option_context_parse(context, &argc, &argv, &error);
> +g_option_context_free(context);
> +
> +if (error) {
if (error != NULL) { } ?
> +g_printerr("Invalid arguments, %s\n", error->message);
> +g_clear_error(&error);
> +return -1;
> +}
>
> -for (;;) {
> -if (-1 == (c = getopt(argc, argv, "-dxhys:f:o:S:")))
> -break;
> -switch (c) {
> -case 'd':
> -debug++;
> -break;
> -case 's':
> -portdev = optarg;
> -break;
> -case 'x':
> -do_daemonize = 0;
> -break;
> -case 'y':
> -x11_sync = 1;
> -break;
> -case 'h':
> -usage(stdout);
> -return 0;
> -case 'f':
> -fx_dir = optarg;
> -break;
> -ca
Re: [Spice-devel] [PATCH v2 2/4] vdagent: move file xfer initialization to a function
On Tue, May 16, 2017 at 07:27:53PM +0200, Victor Toso wrote:
> From: Victor Toso
>
> This patch creates two functions:
> - xfer_get_download_directory()
> - vdagent_init_file_xfer()
>
> The logic should be similar as it was before this patch, taking in
> consideration the global variables fx_open_dir and fx_dir which are
> set from command line.
>
> Signed-off-by: Victor Toso
> ---
> src/vdagent/vdagent.c | 73
> +--
> 1 file changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 9900303..d1e67a2 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -25,6 +25,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -55,6 +56,56 @@ static struct udscs_connection *client = NULL;
> static int quit = 0;
> static int version_mismatch = 0;
>
> +/**
> + * xfer_get_download_directory
> + *
> + * Return path were file-transfer will save the files. Should be freed with
> + * g_free().
> + **/
> +static const gchar *xfer_get_download_directory(bool *open_dir_on_save)
> +{
> +if (open_dir_on_save != NULL)
> +*open_dir_on_save = fx_open_dir != -1 ||
> !vdagent_x11_has_icons_on_desktop(x11);
This assignment is not very easy to read... Does this have to be in
xfer_get_download_directory? I find this unrelated to that function.
> +
> +if (fx_dir != NULL)
> +return fx_dir;
> +
> +if (vdagent_x11_has_icons_on_desktop(x11))
> +return g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
> +
> +return g_get_user_special_dir(G_USER_DIRECTORY_DOWNLOAD);
> +}
> +
> +/**
> + * vdagent_init_file_xfer
> + *
> + * Initialize handler for file xfer and returns true if vdagent_file_xfers is
> + * not NULL.
> + **/
> +static bool vdagent_init_file_xfer(void)
> +{
> +bool xfer_open_dir;
> +const gchar *xfer_dir;
> +
> +if (vdagent_file_xfers != NULL) {
> +syslog(LOG_WARNING, "File-xfer already initialized");
> +return true;
> +}
> +
> +xfer_dir = xfer_get_download_directory(&xfer_open_dir);
> +if (xfer_dir == NULL) {
> +syslog(LOG_WARNING,
> + "warning could not get file xfer save dir, "
> + "file transfers will be disabled");
> +vdagent_file_xfers = NULL;
> +return false;
> +}
> +
> +vdagent_file_xfers = vdagent_file_xfers_create(client, xfer_dir,
> + xfer_open_dir, debug);
> +return (vdagent_file_xfers != NULL);
> +}
> +
> static void daemon_read_complete(struct udscs_connection **connp,
> struct udscs_message_header *header, uint8_t *data)
> {
> @@ -313,26 +364,8 @@ reconnect:
> return 1;
> }
>
> -if (!fx_dir) {
> -if (vdagent_x11_has_icons_on_desktop(x11))
> -fx_dir = "xdg-desktop";
> -else
> -fx_dir = "xdg-download";
> -}
> -if (fx_open_dir == -1)
> -fx_open_dir = !vdagent_x11_has_icons_on_desktop(x11);
> -if (!strcmp(fx_dir, "xdg-desktop"))
> -fx_dir = g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
> -else if (!strcmp(fx_dir, "xdg-download"))
> -fx_dir = g_get_user_special_dir(G_USER_DIRECTORY_DOWNLOAD);
Note that before the help string was:
-f file xfer save dir
ie 'spice-vdagent -f xdg-desktop' was a valid option, and would
resolve to g_get_user_special_dir(G_USER_DIRECTORY_DESKTOP);
Dunno if we want to keep that.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH vd_agent_win] Use sprintf_s instead of sprintf to not crash
>
> >
> > ---
> > vdagent/file_xfer.cpp | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > index de1aea1..de98d50 100644
> > --- a/vdagent/file_xfer.cpp
> > +++ b/vdagent/file_xfer.cpp
> > @@ -113,8 +113,8 @@ void
> > FileXfer::handle_start(VDAgentFileXferStartMessage*
> > start,
> > if (attempt == 0) {
> > strcpy(dest_filename, file_name);
> > } else {
> > -sprintf(dest_filename, "%.*s (%d)%s", int(extension -
> > file_name), file_name,
> > -attempt, extension);
> > +sprintf_s(dest_filename, SPICE_N_ELEMENTS(file_name) +
> > POSTFIX_LEN,
> > + "%.*s (%d)%s", int(extension - file_name),
> > file_name,
> > attempt, extension);
> > }
> > if ((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1, file_path
> > +
> > wlen, MAX_PATH - wlen)) == 0) {
> > vd_printf("failed converting file_name:%s to WideChar",
> > dest_filename);
>
> As far as I know dest_filename is allocate big enough to avoid the
> overflow.
> Did it crash for you or just a warning from the compiler?
>
> This would be easier
>
> sprintf_s(dest_filename, SPICE_N_ELEMENTS(dest_filename),
> "%.*s (%d)%s", int(extension - file_name), file_name,
> attempt, extension);
>
> without computing again the length.
>
Got some discussion with Jakub and Pavel on IRC.
They confirmed that with Fedora 26 and 27 the crash happens.
I tried to do some tests (removed the CreateFile and some calls to have
a test function) with Fedora 25 but didn't manage to reproduce.
Jakub reported that this happens with small names.
Maybe is an issue with MingW with newer Fedora? Worth investigating.
For me sprintf_s is fine. I would just replace the
"SPICE_N_ELEMENTS(file_name) + POSTFIX_LEN" with SPICE_N_ELEMENTS(dest_filename)
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH vd_agent_win v2] Use sprintf_s instead of sprintf to not crash
---
vdagent/file_xfer.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
index de1aea1..e877cca 100644
--- a/vdagent/file_xfer.cpp
+++ b/vdagent/file_xfer.cpp
@@ -113,8 +113,8 @@ void FileXfer::handle_start(VDAgentFileXferStartMessage*
start,
if (attempt == 0) {
strcpy(dest_filename, file_name);
} else {
-sprintf(dest_filename, "%.*s (%d)%s", int(extension - file_name),
file_name,
-attempt, extension);
+sprintf_s(dest_filename, SPICE_N_ELEMENTS(dest_filename),
+ "%.*s (%d)%s", int(extension - file_name), file_name,
attempt, extension);
}
if ((MultiByteToWideChar(CP_UTF8, 0, dest_filename, -1, file_path +
wlen, MAX_PATH - wlen)) == 0) {
vd_printf("failed converting file_name:%s to WideChar",
dest_filename);
--
2.9.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
