Re: [Spice-devel] Issues with Clipboard Grab/Release Callbacks
Hi On Mon, Jun 20, 2022 at 6:09 PM Nick Couchman wrote: > Hello, everyone, > I've posted here, before, with some questions during my journey of trying > to add support for the SPICE protocol to the Guacamole project. The good > news is that I've actually made measurable progress in the implementation - > I can now connect to a SPICE server, and the basics work (mouse and > keyboard - mostly anyway), so I'm moving on to the extra stuff - clipboard, > audio, file transfer, etc. > > In working on the clipboard integration, I'm currently running into an > issue with a couple of the callback functions, specifically the clipboard > grab/release functions, where the number of arguments seems to be > mismatched. According to the documentation, these callbacks should be > called with the following arguments: > > gboolean > user_function (SpiceMainChannel *main, >guint selection, >gpointer types, >guint ntypes, >gpointer user_data) > > types is actually guint32*, but that's not the problem here. > void > user_function (SpiceMainChannel *main, >guint selection, >gpointer user_data) > > I've implemented the callbacks with those arguments, as follows: > > void guac_spice_clipboard_selection_grab_handler(SpiceMainChannel channel, > guint selection, gpointer types, guint ntypes, guac_client* client) > > void guac_spice_clipboard_selection_release_handler(SpiceMainChannel > channel, > guint selection, guac_client* client) > It looks correct. Can you point to your code connecting the signals with the handlers? > > and registered them appropriately. However, if I use them as implemented > above, when the callbacks are triggered, the application segfaults when I > try to access the "guac_client* client" data structure. I used GDB to try > to help track this down, and I noticed that the value of "client" was 0x1, > which looks less like a pointer to a memory location and more like the > number 1. > > So, I decided to add another argument to the callback functions, just > before the client argument: > > void guac_spice_clipboard_selection_grab_handler(SpiceMainChannel channel, > guint selection, gpointer types, guint ntypes, guint extra, > guac_client* client) > > void guac_spice_clipboard_selection_release_handler(SpiceMainChannel > channel, > guint selection, guint extra, guac_client* client) > > Strangely, this works - the client data structure can be referenced, there > are no segfaults (yet), etc. So, I decided to print the values that are > being passed for all of these parameters, and I get the following: > > guacd[100252]: DEBUG: Notifying client of clipboard grab in the guest. > guacd[100252]: DEBUG: Arg: channel: 0x > guacd[100252]: DEBUG: Arg: selection: 1275303536 > guacd[100252]: DEBUG: Arg: types: 0x0001 > guacd[100252]: DEBUG: Arg: ntypes: 1276022924 > guacd[100252]: DEBUG: Arg: extra: 1 > > Weird > I printed them in the format I thought they should be in based on what the > arguments are supposed to be - I probably should have just done all hex. > But, it seems like maybe the "extra" parameter being passed is in front of > the channel, since the channel is showing up as all zeros? > > I was trying to find the code where the callbacks are actually called - > I'm guessing, since this is clipboard integration, it'll be in the vdagent > code somewhere - but I was having trouble tracking that down. > > The callbacks are not directly called, since those a GObject signals. The main channel will call g_signal_emit(). > If anyone has any ideas, I'd appreciate the insight into this - I'm > puzzled by this apparent mismatch in the number of arguments. Also, if it > matters, I'm running CentOS 8 Stream, using Xspice to provide a test SPICE > server, and running spice-vdagentd/spice-vdagent within my X session. Yes, > I know Xspice is unmaintained, but I just needed something simple and that > I didn't have to spend a bunch of time building in order to give me a spice > server to point at, and, so far, this has been pretty reliable. Also, at > its core, it appears to use the same spice-qxl X driver that x11spice uses, > just with a simple Python wrapper script for generating an X config file > and starting the X server/display. So, I think it's still pretty "safe" for > attempting to develop this Guacamole integration - if for some reason you > believe me to be wrong about that, please let me know. > Your testing environment shouldn't be a problem, it's really your client-side code integration with spice-glib that looks broken. -- Marc-André Lureau
[Spice-devel] WIP: usbredir rust bindings
Hi Just to let you know that I started a usbredir binding to Rust: https://gitlab.freedesktop.org/elmarco/usbredir-rs Atm, it compiles with a custom rusb. I have some MR there ( https://github.com/a1ien/rusb/pull/97 & https://github.com/a1ien/rusb/pull/101) See the example: https://gitlab.freedesktop.org/elmarco/usbredir-rs/-/blob/main/usbredirhost/examples/usbredir.rs Atm, I pass an open fd with bash for testing, ex: exec 3<>/dev/tcp/localhost/ target/debug/examples/usbredir '1050:0407' --fd 3 Next, it could be extended this with [--tcp host:port], [--unix path], [-l|--listen]. Then I suppose it will be close to https://gitlab.freedesktop.org/spice/usbredir/-/blob/master/tools/usbredirect.c (Note that the Rust code doesn't use poll(), since it isn't portable. See https://libusb.sourceforge.io/api-1.0/group__libusb__poll.html#libusb_pollmain. The libusb/usbredir APIs don't make it easy to handle events..) -- Marc-André Lureau
Re: [Spice-devel] github.com/spice
Hi Luke On Thu, Jul 22, 2021 at 11:11 AM Luke Kim wrote: > Hi! > > > > I’m the founder of a new AI startup called Spice AI. > > > > We noticed that the SPICE project at github.com/spice looks like it might > not be in use anymore, and that you might be using GitLab. > > > > If that’s the case, would you consider a conversation on transferring the > GitHub org over to us? > > > We reserved the name on github a long time ago. There are already several projects called Spice. It would be better if you named your organization on github "spiceai" instead. It would avoid the confusion. Ideally github.com/spice should redirect to the various "spice" projects, in a wiki-index fashion, but I don't think we can do that yet. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] spice-vdagent user systemd unit
Hi On Fri, Jul 16, 2021 at 7:25 PM Rex Dieter wrote: > Just a quick followup to old thread > "Fedora 34 guests can no longer paste from host" > https://lists.freedesktop.org/archives/spice-devel/2021-April/052351.html > > The problem is purely relying on autostart which can be buggy in this > case, see downstream bug: > https://bugzilla.redhat.com/show_bug.cgi?id=1951580 > and relevant upstream bug, > https://github.com/systemd/systemd/issues/18791 > > One workaround is to provide a systemd user unit to run spice-vdagent > also/instead. Here's the first iteration I came up with: > > https://src.fedoraproject.org/rpms/spice-vdagent/blob/rawhide/f/spice-vdagent.service > (pretty simple) > > Feedback is welcome, and I'd encourage you to adopt this upstream too. > Thanks. > > That looks reasonable to me. X-GNOME-Autostart-Phase=WindowManager was not meant to be a generic solution anyway. It seems reasonable to have a systemd session solution nowadays. Even if both compete at startup, only one should stay. Can you make a merge request on gitlab? -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Fedora 34 guests can no longer paste from host
Hi On Tue, Apr 20, 2021 at 1:56 PM Germano Massullo wrote: > Good day, I always used with success a Fedora 33 KDE host > (qemu/kvm/libvirt+virt-manager) and a F33 KDE guest, and I have always been > to copy paste from host to guest and viceversa > > Now I tested two Fedora 34 Beta guests: > - KDE spin > - Workstation (GNOME) > and both of them fail to paste text and files from the host. spice-vdagent > version is the same on both F33 and F34 guests: 0.21.0. Host machine has > spice-gtk3-0.39-1.fc33.x86_64 > All machines mentioned in this bugreport are Xorg based. > > Hereunder I attach output of > $ SPICE_DEBUG=1 virt-viewer --connect > qemu:///systemhttps://germano.fedorapeople.org/bugreport/spice/spice_debug.txt > > I think the main hint is: ../src/channel-main.c:1590 agent connected: no I did a quick f34 beta VM check (GNOME), a few days ago. I also reached a similar situation at some point, but it was gone before I could understand the reason (probably after update & reboot). Either it was fixed elsewhere or we will have to investigate further if it happens randomly. Anyway thanks for the report # virsh dumpxml fedora34gnomehttps://germano.fedorapeople.org/bugreport/spice/virsh_dumpxml.txt > > What can be the problem? > Thank you > > > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel > -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Adding SPICE support to Guacamole - spice-glib
Hi On Fri, Jan 8, 2021 at 12:07 AM Nick Couchman wrote: > Hey, everyone, > I'm a contributor to the Guacamole project, and am currently working on > adding support for the SPICE protocol to Guacamole. If you haven't heard of > Guacamole, it is a protocol and associated software components for making > various remote desktop interfaces available via native HTML5 in browsers. I > realize that there is already a spice-html5 client, but, for various > reasons, we'd like to add the SPICE support to Guacamole, as well. > > Within Guacamole, the clients for the individual protocols are implemented > in a daemon - guacd - that is written in C. It accesses the various remote > desktop protocols that we support (VNC, RDP, SSH, and Telnet, today) and > does the translation to the Guacamole protocol. So, I am attempting to > write the SPICE client for this using the spice-glib library. > > I'm running into an issue with the code I've written thus far. I've > followed API documentation and some code examples, and I have it to where > I'm configuring all of the required pieces - hostname/IP, tls-port, > password, etc. I've set up the handlers for the session (channel-event) and > for the channels. When I attempt to connect, the SPICE client starts up, > the session starts/connects, and I see the main channel get created, but > the connection never progresses after that. As far as I can tell, it > doesn't even try any sort of connection to the SPICE server - I don't see > any network traffic going to the SPICE server, and it never progresses past > that point. I'm not seeing any errors - nothing that indicates it's tried > and failed, or is expecting additional input, or anything like that. > > I'm happy to share code if anyone is interested in looking at what I've > done so far, or if anyone has any generic hints as to what I might check or > resources that are helpful in writing a C-based client for SPICE, I'd > greatly appreciate the insight and help. I'm sure there's something > reasonably simple that I'm not doing, or doing out of order, but I'm a bit > stumped at the moment. > Spice uses multiple connections which are called channels for the various streams. You need to get and listen for available channels on the SpiceSession. Have you looked at some of the tools in spice-gtk source tree, for example spicy-screenshot.c ? https://gitlab.freedesktop.org/spice/spice-gtk/-/blob/master/tools/spicy-screenshot.c, this should give you a simple way to get started. You will need to handle the SpiceDisplayChannel, SpiceCursorChannel and SpiceInputsChannel to have basic UI remoting iirc. Hope that helps -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] 6 commits - meson.build src/map-file src/spice-glib-sym-file src/spice-gtk-session.c src/spice-session.c src/spice-session.h src/spice-session-priv.h
Hi On Wed, Sep 9, 2020 at 6:45 PM Jakub Janku wrote: > On Wed, Sep 9, 2020 at 4:36 PM Frediano Ziglio wrote: > > > > > On Wed, Sep 9, 2020 at 4:16 PM Frediano Ziglio > wrote: > > > > > > > > > > commit 4b9092b96b8da946ff3d17922b0fcf225c5dc81f > > > > > > > > > > > Author: Jakub Janků < jja...@redhat.com > > > > > > > > > > > > Date: Sat May 23 16:28:52 2020 +0200 > > > > > > > > > > > > > > > session: make spice_session_get_webdav_server() public > > > > > > > > > > > > > > > It will be necessary to access the webdav server from > > > > > > spice-gtk-session.c > > > > > > > > > > > which isn't compiled with spice-session-priv.h, so make > > > > > > > > > > > spice_session_get_webdav_server() public. > > > > > > > > > > > > > > I haven't looked at the whole series. Wouldn't it make sense to > make it a > > > > > read-only property instead? > > > > > > > > It sounds reasonable for me. > > > > Jakub ? > > > > > > > > > > I agree. > > > > > > Revert the commits please. I'll reopen the merge request once I have it > > > ready. > > > > > > Cheers, > > > Jakub > > > > > > > To be honest I don't see the need to revert commits, it's just a change > > from public to private. > > Ok, so should I open a separate MR? > > To make sure that I didn't misunderstand it: the suggestion is to keep > spice_session_get_webdav_server() private and install a new > SpiceSession read-only property "webdav", correct? > > yes (the main motivation is to avoid adding new library symbols, and properties can be looked up at runtime, which may avoid bumping dependencies in some cases) -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] 6 commits - meson.build src/map-file src/spice-glib-sym-file src/spice-gtk-session.c src/spice-session.c src/spice-session.h src/spice-session-priv.h
705,6 +724,9 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> return;
> }
>
> +g_clear_pointer(>atoms[selection], g_free);
> +s->n_atoms[selection] = 0;
> +
> if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> if (s->clip_grabbed[selection]) {
> /* grab was sent to the agent, so release it */
> commit 852b847c868a199b5127644ca689f8a7d70fbda1
> Author: Jakub Janků
> Date: Fri May 29 17:57:51 2020 +0200
>
> spice-gtk-session: add clipboard_get_open_webdav()
>
> File copy functionality will only be enabled when there is an
> open
> webdav channel.
>
> Signed-off-by: Jakub Janků
> Acked-by: Frediano Ziglio
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 48058c7..2b86616 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -565,6 +565,32 @@ static gpointer free_weak_ref(gpointer data)
> return object;
> }
>
> +#ifdef HAVE_PHODAV_VIRTUAL
> +static SpiceWebdavChannel *clipboard_get_open_webdav(SpiceSession
> *session)
> +{
> +GList *list, *l;
> +SpiceChannel *channel = NULL;
> +gboolean open = FALSE;
> +
> +g_return_val_if_fail(session != NULL, NULL);
> +
> +list = spice_session_get_channels(session);
> +for (l = g_list_first(list); l != NULL; l = g_list_next(l)) {
> +channel = l->data;
> +
> +if (!SPICE_IS_WEBDAV_CHANNEL(channel)) {
> +continue;
> +}
> +
> +g_object_get(channel, "port-opened", , NULL);
> +break;
> +}
> +
> +g_list_free(list);
> +return open ? SPICE_WEBDAV_CHANNEL(channel) : NULL;
> +}
> +#endif
> +
> static void clipboard_get_targets(GtkClipboard *clipboard,
>GdkAtom *atoms,
>gint n_atoms,
> commit c1b5433815e5cd7683671d33a0d579b7b185efe8
> Author: Jakub Janků
> Date: Mon Jun 29 19:40:25 2020 +0200
>
> build: require GLib 2.52+
>
> This adds g_uuid_string_random()
> which is necessary for the following file copy
> functionality.
>
> Signed-off-by: Jakub Janků
> Acked-by: Frediano Ziglio
>
> diff --git a/meson.build b/meson.build
> index 1c4e9d9..7ade460 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -89,7 +89,7 @@ endforeach
> #
> # check for mandatory dependencies
> #
> -glib_version = '2.46'
> +glib_version = '2.52'
> glib_version_info = '>= @0@'.format(glib_version)
> pixman_version = '>= 0.17.7'
>
> commit 979b752b24d6f8d7089a23760fd5adda18f0e7ed
> Author: Jakub Janků
> Date: Sat May 23 13:40:39 2020 +0200
>
> build: define HAVE_PHODAV_VIRTUAL if phodav >= 2.5
>
> Phodav 2.5 brings PhodavVirtualDir API needed for the
> file copy and paste functionality.
>
> If the library version is not sufficient, this new feature
> will be disabled, but the standard shared folders can still
> be used.
>
> Signed-off-by: Jakub Janků
> Acked-by: Frediano Ziglio
>
> diff --git a/meson.build b/meson.build
> index 6bbb4a8..1c4e9d9 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -177,14 +177,17 @@ endif
>
> # webdav
> spice_gtk_has_phodav = false
> -d = dependency('libphodav-2.0', required: get_option('webdav'))
> -if d.found()
> - spice_glib_deps += d
> +phodav_dep = dependency('libphodav-2.0', required: get_option('webdav'))
> +if phodav_dep.found()
> + spice_glib_deps += phodav_dep
>d = dependency('libsoup-2.4', version : '>= 2.49.91', required:
> get_option('webdav'))
>if d.found()
> spice_glib_deps += d
> spice_gtk_config_data.set('USE_PHODAV', '1')
> spice_gtk_has_phodav = true
> +if phodav_dep.version().version_compare('>= 2.5')
> + spice_gtk_config_data.set('HAVE_PHODAV_VIRTUAL', '1')
> +endif
>endif
> endif
>
> commit 4b9092b96b8da946ff3d17922b0fcf225c5dc81f
> Author: Jakub Janků
> Date: Sat May 23 16:28:52 2020 +0200
>
> session: make spice_session_get_webdav_server() public
>
> It will be necessary to access the webdav server from
> spice-gtk-session.c
> which isn't compiled with spice-session-priv.h, so make
> spice_session_get_webdav_server() public.
>
I haven't looked at the whole series. Wouldn't it make sense to make it a
read-only property instead?
>
> Signed-off-by: Jakub Janků
> Acked-by: Frediano Ziglio
>
> diff --git a/src/map-file b/src/map-file
> index acdd38f..86f371d 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -144,6 +144,7 @@ spice_session_new;
>
Re: [Spice-devel] Is it possible to put spice channels into different threads?
Hi On Fri, Jun 12, 2020 at 12:57 PM 陈炤 wrote: > Hi, > > Spice-gtk is now using co-routine to handle different channel connections. > When a channel is handling data, other channels would have to wait, rather > than handling synchronously. That would bring us following issues: > 1. If some less important channels (like usb channels) are transfering > big data, important channels (main-channel, display-channel,input-channel) > will be affected. > 2. When receiving big data like file transfering(G_IO_IN), send event > (G_IO_OUT) will not be triggered. > 3. Flow control between different channels will be hard to do. > > Is is possible(and make sense) to put channels into different threads so > they can synchronously receive & send msg, without affect each other? > > Switching to threads would be possible, but that wouldn't help in the situation you describe, as you are very likely bound on IO. Using several threads would actually create more problems to synchronize and schedule the different channels. Io operations in coroutines are non-blocking, so they shouldn't affect other spice-gtk task. If you however observe a blocking CPU-task in some channel, this may affect the performance of other channels. But in general, except for video/image decoding which may be done in a separate thread, the client side doesn't do much work. USB, clipboard and file sharing may use large amounts of data, and we rely on the glib source and kernel to prioritize channels: this isn't great in some cases and may receive improvements. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Using QXL & Spice with Windows Host & QEMU
Hi On Mon, Jun 8, 2020 at 6:00 PM wrote: > Hello everyone, > > when using Qemu 4.2.0 with a Windows host, qemu-system-x86_64 reports: > "QXL VGA not available" > > I used the following Qemu args for testing: > qemu-system-x86-x64.exe -accel hax -machine q35 -m 4GB -cdrom > xubuntu-20.04-desktop-amd64.iso > > Using either "-vga stdt" or "-vga vmware" works. The performance is just > not that good. > > Do I have a chance to get that up and running with a Windows 10 host and > Qemu? > You better ask the question on qemu-de...@nongnu.org ML, or report a bug directly to https://bugs.launchpad.net/qemu/ ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
Hi On Thu, May 14, 2020 at 2:32 PM Marc-André Lureau < marcandre.lur...@gmail.com> wrote: > Hi > > On Mon, May 11, 2020 at 12:16 PM Marc-André Lureau < > marcandre.lur...@redhat.com> wrote: > >> Hi, >> >> About "Move code to C++": >> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 >> >> I would like to know how the decision happened. As long as I have been >> involved in the Spice project, we had open discussions and did >> mandatory review for anything non-trivial. >> >> This change is substantial, and impacts the work and contribution of >> others. Where did the discussion happen? Who reviewed the code >> changes? >> >> > Since no real discussion nor public review took place, I propose to revert > the change and add a HACKING file to make the rules clear. See: > > https://gitlab.freedesktop.org/spice/spice/-/merge_requests/91 > > Based on the discussion there, and the fact that a majority of developper are willing to take the change, I closed the MR. Fwiw, beside the fact that the change wasn't done with review nor discussion (which on its own is detrimental), I think using C++ is wrong: - C++ is much more complex than C, and fewer developper know it. A good C++ developper is an even better C developper. - in our common ecosystem, QEMU, Linux, the GNOME desktop, are 90% C projects, and perhaps? 1% C++? (I don't know a single C++ system/core project tbh - only apps like KDE/LibreOffice/Firefox) - C++ projects are switching away from it (ex Firefox), I don't know of a core/system project talking about switching to C++ these days. - switching to C++ is going to be costly, as it is endless, and it will leave a bitter taste of previous/existing C. We could have used that time for other things... I would be pleasantly surprised if such refactoring would lead to regressions. - C has better interoperability story than C++, especially because the code was GObjectified. We could have added/exported more friendly API/bindings for GNOME easily. Overall, it's a waste of effort to me. What is the overall direction of the project? Except a few language improvements for a lower number of developpers, what does C++ bring us that we couldn't do with C for our users? Nevertheless, I will try to invest some time reviewing the changes. I hope we can agree on better project rules and directions for the future. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Brainstorming help with x11spice on socket permissions across users
Hi On Tue, May 26, 2020 at 3:55 PM Jeremy White wrote: > Hi all, > > I'm trying to get x11spice and spice-html5, at least as packaged for > Fedora, into a pretty much 'turn key' state. > > I've got 3 use cases. The first is user A sharing their current > desktop, either for themselves, or to get help. That case is largely > done, imho, modulo some documentation and perhaps some streamlining. > I didn't know you could do that. I suppose the solution is X11 only? It would be nice to have gnome-remote-desktop integration. Though GNOME seems more interested to support RDP these days (having a glib/gobject server library would certainly help them to consider Spice, *hint* ;) The second is user A getting access to a new session for themselves. I > don't feel blocked on this case; the work should be straight forward, if > fiddly (I may regret those words; doing a secure 'su' like function out > of apache may be harder than I think). > Multiple user session is tricky. Afaik, this is mostly used for desktop development. The instructions to setup such environmnent change over time and desktop. Did I miss something? What's the use case? > The 3rd case, however, has me troubled. This is the case that user A > (potentially apache) starts x11spice which then does an xdmcp request to > gdm, and eventually supports a log in by user B. This makes it > challenging to provide a way for user B to launch a spice agent or a > pulseaudio daemon and have it securely connect back to the spice process > started by user A. The approach I've used in the past is to have a > privileged binary use information from an X atom to adjust socket > permissions. But that feels unsatisfying, and it seems to me that this > is an area with a lot of modern thinking that I've largely missed. > > As an added complexity, in the ideal case, you have a vdagent running as > user A during the login process, which knows to reap itself and give way > to a vdagent launched by user B. > > I was hoping that others would have modern instincts on how to more > correctly implement the third use case. Clue bats or other ideas welcome. > This is systemd/desktop territories, and I don't know what would be the best way to do all that. I would suggest you ask the gnome-remote-desktop & systemd/logind developpers, or other desktop developpers how they plan or not to solve it. cheers -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] SPICE: changing the merge rules - a proposal
Hi On Tue, May 19, 2020 at 11:10 AM Frediano Ziglio wrote: > > > > Hi, > >the community around the SPICE project always tried to follow one > > fundamental, implicit rule for accepting code contributions to the > > project: every merge request (beside trivial patches) should be reviewed > > and acked at least by one before getting merged. > > While everyone agrees with this fundamental rule, the actual status of > > some SPICE projects makes the rule impractical to let the project move > > forward. > > Let's consider the spice/spice project as an example: the number of > > contributions is very low, both on the commit side (only 4 different > > contributors with more than 1 commit from the beginning of the year, and > > a single contributor with 90% of commits) and on the review side (in the > > last 40 merge requests before the C++ switch one, 21 had no comments). > > The x11spice project is another example: we have only 4 contributors > > from the beginning of the year (and a single contributor holding 70% of > > the commits) and the reviews on the gitlab merge requests have been > > provided by two people only, each one reviewing the merge requests of > > the other. > > For the sake of having the projects being able to move forward with a > > reduced number of contributors/reviewers, the proposal is to *allow* a > > maintainer to merge a Merge Request without an explicit ack if the three > > following conditions are met: > > 1) The Merge Request has been pending for at least 3 weeks without > > getting new comments > > 2) The Merge Request submitter has kept asking a review on a weekly basis > > 3) There are no pending nacks on the Merge Request > > > > Note that having patches reviewed would still be the preferred way. If > > at any time the number of contributors would raise again, we can switch > > back to the mandatory review rule. Until then the priority is to allow > > the project to move forward. > > > > What do you think? Please share your thoughts and/or contribute with > > your own ideas. > > > > Thanks > > > > Francesco > > > > Hi, >I agree on the proposal. This problem has been going on for years now. > The issue involve all repositories. The active part of community dealing > with code review is very low. > All open-source projects are under-staffed. Spice has had a number of paid developpers, and a number of paid developpers who can help when it is needed. This is better than a lot of projects. I think 3 weeks are perfectly fine. We should be in a trustful community, > I expressed my anxiousness about that short period. This is not going to be healthy to put such constrain. If we were unhappy about the time it took to review a MR, we would need first to prioritize our work better, and reaching out for help. Not rush the changes, please. if somebody is very busy in daily activity or on holidays whomever remains > should be in charge. Is not that is a person in a team goes on holiday all > the work of the team is impeded. > I don't think such "blocking" factor exist, because the Spice code base is large, and the domain as well. But in such exceptional circonstances, we can easily either find a consensus for an exception, or help each other to unblock the situation. thanks -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] SPICE: changing the merge rules - a proposal
Hi On Fri, May 15, 2020 at 7:06 PM Francesco Giudici wrote: > Hi, >the community around the SPICE project always tried to follow one > fundamental, implicit rule for accepting code contributions to the > project: every merge request (beside trivial patches) should be reviewed > and acked at least by one before getting merged. > While everyone agrees with this fundamental rule, the actual status of > some SPICE projects makes the rule impractical to let the project move > forward. > I wasn't aware of a maintenance problem. Perhaps we should first list the projects that have maintenance issues & discuss our options, before changing the common rule. Let's consider the spice/spice project as an example: the number of > contributions is very low, both on the commit side (only 4 different > contributors with more than 1 commit from the beginning of the year, and > a single contributor with 90% of commits) and on the review side (in the > last 40 merge requests before the C++ switch one, 21 had no comments). > You are omitting the passive reviewers. I consider myself as one of them. If you need people to be more proactive, you could at least reach me & probably others past contributors. The x11spice project is another example: we have only 4 contributors > from the beginning of the year (and a single contributor holding 70% of > the commits) and the reviews on the gitlab merge requests have been > provided by two people only, each one reviewing the merge requests of > the other. > As projects become more specific/marginal, it's clear that the number of maintainers is lower. Yet, we have those projects under the same umbrella, and I don't think they should be treated differently. 2 active developers/maintainers can be very healthy, I would say. So do we have a maintenance issue with x11spice? Do we want to move the project out of the "Spice-space" projects for ex? What is the problem exactly? For the sake of having the projects being able to move forward with a > reduced number of contributors/reviewers, the proposal is to *allow* a > maintainer to merge a Merge Request without an explicit ack if the three > following conditions are met: > 1) The Merge Request has been pending for at least 3 weeks without > getting new comments > 2) The Merge Request submitter has kept asking a review on a weekly basis > 3) There are no pending nacks on the Merge Request > > It's hard to define a delay to bypass a reviewing & consensus rule. In general, it should really be frowned upon. There is clearly more than one person interested and using Spice. If the issue is important, it should be fairly easy to get someone else to look at the issue in a timely manner. If not, there are probably more important/interesting things to do to get other interested. If Spice is good enough for our users and interested parties, then it may be risky to change it in wild directions without their approval. But 3 weeks is way too short. You could have more important work, family issue, get sick and go on holiday, all happening each after the other. If we need you to review some code, because you have the best experience, we should wait. If really we want a delay, I would argue waiting 3 months minimum (there might be exceptional circumstances, but they are exception, not the rule). And during that time, the contributor should have attempted multiple time to involve people, by different means (at least ML, irc and gitlab issue+MR - eventually try a conf call to explain the motivation, present the work differently etc, complain about the Spice project laziness on public blog if need be etc). Note that having patches reviewed would still be the preferred way. If > at any time the number of contributors would raise again, we can switch > back to the mandatory review rule. Until then the priority is to allow > the project to move forward. > > What do you think? Please share your thoughts and/or contribute with > your own ideas. > Thanks, but I think the trivial rule is already very subtle and generally disapproved (fwiw, I am still in favor of a subjective trivial rule), so I don't think this proposal would work. We have to grow a community, by convincing people and doing interesting work. Not by doing personal thing, and then leave the pieces behind, because we did it alone and didn't gather others. Perhaps Spice is doing the job. Perhaps Spice needs to define new objectives. These are interesting topics that I believe people would want to discuss and participate. If not, we are doing it wrong. thanks -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
Hi On Thu, May 14, 2020 at 2:32 PM Marc-André Lureau < marcandre.lur...@gmail.com> wrote: > Hi > > On Mon, May 11, 2020 at 12:16 PM Marc-André Lureau < > marcandre.lur...@redhat.com> wrote: > >> Hi, >> >> About "Move code to C++": >> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 >> >> I would like to know how the decision happened. As long as I have been >> involved in the Spice project, we had open discussions and did >> mandatory review for anything non-trivial. >> >> This change is substantial, and impacts the work and contribution of >> others. Where did the discussion happen? Who reviewed the code >> changes? >> >> > Since no real discussion nor public review took place, I propose to revert > the change and add a HACKING file to make the rules clear. See: > > https://gitlab.freedesktop.org/spice/spice/-/merge_requests/91 > I propose a spice-space page derived from libvirt "Project governance": https://gitlab.freedesktop.org/spice/spice-space-pages/-/merge_requests/18 (readable version https://gitlab.freedesktop.org/spice/spice-space-pages/-/blob/9a1907de182f9f64450a7348e9e6a1423dfa580a/governance.rst ) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
Hi On Mon, May 11, 2020 at 12:16 PM Marc-André Lureau < marcandre.lur...@redhat.com> wrote: > Hi, > > About "Move code to C++": > https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 > > I would like to know how the decision happened. As long as I have been > involved in the Spice project, we had open discussions and did > mandatory review for anything non-trivial. > > This change is substantial, and impacts the work and contribution of > others. Where did the discussion happen? Who reviewed the code > changes? > > Since no real discussion nor public review took place, I propose to revert the change and add a HACKING file to make the rules clear. See: https://gitlab.freedesktop.org/spice/spice/-/merge_requests/91 Frediano, I suggest you open a thread about switching the code to c++. thanks -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Spice status
Hi On Thu, May 14, 2020 at 1:06 PM Roland Sassen wrote: > can you please give me a short update about Spice? > > 1status quo > 2roadmap > 3how many people work on the project > It's not easy to answer such question, but it's an open-source project. You may have a measure of the work being done by looking at the gitlab pages/issues, mailing list activity and the git repository statistics. https://gitlab.freedesktop.org/spice 4support for mobile phones > Clients exist on Android. I am not aware of MacOS solution. I am not aware of remoting a phone UI either. > 5video from mobile phones > What do you mean? Playback of video on mobile phones should work. Recording of video isn't probably possible, since afaik the only solution with Spice today is to use USB passthrough. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] virtual gl
Hi On Wed, May 13, 2020 at 5:30 PM Roland Sassen wrote: > Hello, is it possible to use spice together with virtual GL? > If you are talking about VirtualGL (https://virtualgl.org/), Spice doesn't provide 3d rendering itself. However, you may use 3d/opengl in the guest/remote, by various means. Either via cpu rendering, gpu passthrough, or virgl. Thus, you can use VirtualGL together with Spice, but the 2 solutions will remain distinct. They offer different trade-offs. Spice is designed to work at the display/monitor level, whatever the OS, while VirtualGL is more an X11 application-level solution, afaik. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
Hi On Tue, May 12, 2020 at 10:08 PM Francesco Giudici wrote: > > > > On 12/05/20 19:13, Marc-André Lureau wrote: > > Hi > > > > On Tue, May 12, 2020 at 5:07 PM Francesco Giudici > > wrote: > >> > >> > >> > >> On 12/05/20 11:24, Daniel P. Berrangé wrote: > >>> On Mon, May 11, 2020 at 12:16:24PM +0200, Marc-André Lureau wrote: > >>>> Hi, > >>>> > >>>> About "Move code to C++": > >>>> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 > >>>> > >>>> I would like to know how the decision happened. As long as I have been > >>>> involved in the Spice project, we had open discussions and did > >>>> mandatory review for anything non-trivial. > >>>> > >>>> This change is substantial, and impacts the work and contribution of > >>>> others. Where did the discussion happen? Who reviewed the code > >>>> changes? > >>> > >>> Looking at that merge request, it is pretty surprising to see a 100 > >>> patch long series merged with no review comments visible on the code > >>> from other maintainers. > >>> > >>> Regards, > >>> Daniel > >>> > >> > >> I see your points: a proper discussion and fair review on the branch > >> would have been the way to go. > >> To have a fair overall picture btw, I think we should also consider some > >> other points: > >> $> git shortlog --since 01/01/2019 -s -n > >> 411 Frediano Ziglio > >> 29 Victor Toso > >> 20 Uri Lublin > >> 14 Francois Gouget > >> 11 Christophe Fergeau > >> 10 Snir Sheriber > >>6 Eduardo Lima (Etrunko) > >>6 Jonathon Jongsma > >>6 Kevin Pouget > >>6 Lukáš Hrázký > >>5 James Le Cuirot > >>3 Thiago Mendes > >>2 Rosen Penev > >>1 Benjamin Tissoires > >>1 Christian Ehrhardt > >>1 Douglas Paul > >>1 Gilmar Santos Jr > >>1 Jeremy White > >>1 worldofpeace > >>1 谢 昆明 > > > > To be "fair", the number of commits in spice server alone over 1.5y is > > not a very good metric. Certainly, Frediano's work is consequent, > > thank you, but it's also part of his job afaik. > My point here is that in the last 1.5y there was very little > contribution to the project apart from Frediano. Same on the review > side. In this context, I can understand why the branch was merged. I > have my opinion, but here I don't want to say he did it right or he did > it wrong: I'm just saying I can understand why he did. I don't understand the justification, honestly. The spice server is not overwhelmed with MR, and there is no rush to switch to C++. > > > > >> > >> Frediano's branches don't get much reviews (if any... see the full list > >> of merged/closed MR in gitlab for the spice/spice project). I think we > >> all agree that his intention is good, which is to just move the project > >> forward. Wondering who would have looked into his 100 patches branch to > >> do a fair review in a reasonable time-frame. > >> I feel (at least partially) guilty for this situation. > >> > >> That said... at this point the branch has been merged. What are the > >> proposals now? > >> > >> Draft a more formal review/commit policy? What if a branch doesn't get a > >> fair review in an acceptable time-frame? Who will have the last word if > >> unanimity is not reached? > > > > What does an acceptable time-frame mean? If the work is important, the > > project maintainers should do a review in a timely manner. If nobody > > has enough time to review changes, isn't it because we, collectively, > > have more important things to do? The contributor who proposed such a > > change in the first place should realize that. > > Well, agreeing on what is "important" is not easy, as it is personal. > Any contribution is important to the submitter ;-) It's a collective work, not somebody's project with its personal priority or wishes. > But my point is, let's make the rules clearer, so to try to avoid > similar situations in the future. We have had rules for years. Anything non-trivial has mandatory reviews. And in practice, we try to go through review all the time. > > > >> > >> Do you want
Re: [Spice-devel] About decisions and reviews
Hi On Tue, May 12, 2020 at 5:07 PM Francesco Giudici wrote: > > > > On 12/05/20 11:24, Daniel P. Berrangé wrote: > > On Mon, May 11, 2020 at 12:16:24PM +0200, Marc-André Lureau wrote: > >> Hi, > >> > >> About "Move code to C++": > >> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 > >> > >> I would like to know how the decision happened. As long as I have been > >> involved in the Spice project, we had open discussions and did > >> mandatory review for anything non-trivial. > >> > >> This change is substantial, and impacts the work and contribution of > >> others. Where did the discussion happen? Who reviewed the code > >> changes? > > > > Looking at that merge request, it is pretty surprising to see a 100 > > patch long series merged with no review comments visible on the code > > from other maintainers. > > > > Regards, > > Daniel > > > > I see your points: a proper discussion and fair review on the branch > would have been the way to go. > To have a fair overall picture btw, I think we should also consider some > other points: > $> git shortlog --since 01/01/2019 -s -n > 411 Frediano Ziglio > 29 Victor Toso > 20 Uri Lublin > 14 Francois Gouget > 11 Christophe Fergeau > 10 Snir Sheriber > 6 Eduardo Lima (Etrunko) > 6 Jonathon Jongsma > 6 Kevin Pouget > 6 Lukáš Hrázký > 5 James Le Cuirot > 3 Thiago Mendes > 2 Rosen Penev > 1 Benjamin Tissoires > 1 Christian Ehrhardt > 1 Douglas Paul > 1 Gilmar Santos Jr > 1 Jeremy White > 1 worldofpeace > 1 谢 昆明 To be "fair", the number of commits in spice server alone over 1.5y is not a very good metric. Certainly, Frediano's work is consequent, thank you, but it's also part of his job afaik. > > Frediano's branches don't get much reviews (if any... see the full list > of merged/closed MR in gitlab for the spice/spice project). I think we > all agree that his intention is good, which is to just move the project > forward. Wondering who would have looked into his 100 patches branch to > do a fair review in a reasonable time-frame. > I feel (at least partially) guilty for this situation. > > That said... at this point the branch has been merged. What are the > proposals now? > > Draft a more formal review/commit policy? What if a branch doesn't get a > fair review in an acceptable time-frame? Who will have the last word if > unanimity is not reached? What does an acceptable time-frame mean? If the work is important, the project maintainers should do a review in a timely manner. If nobody has enough time to review changes, isn't it because we, collectively, have more important things to do? The contributor who proposed such a change in the first place should realize that. > > Do you want to do a post-merge review to consider reverting the commits? > Do you want to have a detached "C" branch with the former code to be > kept as a "stable C" one? > Or what else? I am a past contributor to the Spice server, but I regularly have to read or debug the code. I keep co-maintaining the spice-gtk client, although it seems others are doing a pretty good job at helping me. Is there a problem with the Spice server maintenance? I don't know. What do you want me to say... I am disappointed by this change, the nature of the change, switching to c++, and the way it happened. It doesn't reflect practices I like in open-source projects and the way the Spice project has been developed in the past. Overall, the way it happened is detrimental to the project imho, and I would indeed revert the change if nobody reviewed it openly. The spice server should not to be taken so lightly, it doesn't deserve it either. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] About decisions and reviews
Hi On Tue, May 12, 2020 at 5:25 PM Frediano Ziglio wrote: > > > > > Hi, > > > > About "Move code to C++": > > https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 > > > > I would like to know how the decision happened. As long as I have been > > involved in the Spice project, we had open discussions and did > > mandatory review for anything non-trivial. > > > > This change is substantial, and impacts the work and contribution of > > others. Where did the discussion happen? Who reviewed the code > > changes? > > > > thanks > > > > Hi, > your description of the project looks a bit unrealistic to me. > > About 6 months ago we moved to Gitlab for code reviews. The change was > announced to the ML which was the old way of doing. The procedure with > Gitlab is more or less: > - you prepare and post a MR to Gitlab (previously you would send a mail); > - people wanting to review should be subscribed to Gitlab, receive a > notification and they can decide to open discussions, comment or propose > changes (previously they had to reply to mails); > - code is changed based on discussions, comments and proposals; > - there is some final acknowledge of the MR, MR is merged. > > Now: > - MR was opened in the public, questions and comments were on the cover > letter; > - people had the time to comments and discussions. Only one public > discussion raised, correctly addressed and marked as solved, no > other comments following for a month so MR was correctly reviewed; > - there was some final acknowledge so it was merged. "MR was correctly reviewed", without a single comment on code over ~100 patches? > > The question here is "was it enough"? Honestly this reminded my the > "epsilon" constant in Math. Something really close to zero... but still > positive! I raised the problem of the poor reviews multiple time, trying > to push people. I was discouraged multiple times. Correctly contributions > are made voluntarily and it's not good to force people to work voluntarily. > In a democracy people that does not vote are basically not counted. > > What is your expectation? Every hunk addressed? What is the frame time? > Maybe finish reading and commenting the cover letter for the end of > the year? It's not a question of time. If something needs to be addressed, we put the resources behind. Does switching to c++ fits that category? Probably not (certainly not if you ask me). > > Some time ago, when reviews were still done with ML, but the issue was clearly > present I tried to stop reviewing myself. The result was clear. Most of the > external code contributions were simply ignored, no replies at all. > Some reached almost 2 months without any reply... then I started replying > again! > The situation has been like this for years and it get worse and worse. > Today Victor linked a 3 years old proposal for some mouse enhancement. Let's not point fingers, but there are probably many reasons for that. It's not a reason to take a many year project, decide what it's good or not in private, and push unreviewed commits. > > The definition of contributor involve contributions. Keeping apart non-code > contributions what are the code contributions? Surely new code, reviews, > comments and proposal on code. Now, if people does not review, open or > continue discussion or proposal are they real contributors? How many > stable contributors can Spice project state? You are going to scare even more contributors, by deciding by yourself or in private and not involving them. > Let's look at the major feature in the last Spice server release: > Windows support and WebSockets. Beside being myself strong contributing to > both (first was just a rainy weekend challenge) the second had a bit too > long history. > The patches for WebSockets were sent years ago reviewed quickly without much > attempts to help the writer solving the raised issues. Some proposal were > pretty unrealistic (like "why don't you rewrite everything?"). This was not > even the first attempt to add this feature to Spice. The total realizations > took 7 years. I would saying not a great way to involve new contributors. > I want the project to evolve more quicker. There are many contributions, in many projects, that can take years to be merged. And many contributions never get merged. Spice is no exception. If there are enough interest, there are enough people behind it to polish the work. If not, then you cannot put the project at risk by rushing work. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] About decisions and reviews
Hi, About "Move code to C++": https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62 I would like to know how the decision happened. As long as I have been involved in the Spice project, we had open discussions and did mandatory review for anything non-trivial. This change is substantial, and impacts the work and contribution of others. Where did the discussion happen? Who reviewed the code changes? thanks ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] src/channel-main.c
||
> - strlen((const char*)info->cert_subject_data) == 0) {
> +strlen((const char*)info->cert_subject_data) == 0) {
> /* only verify hostname if no cert subject */
> g_object_set(mig->session, "verify", SPICE_SESSION_VERIFY_HOSTNAME,
> NULL);
> } else {
> @@ -2690,7 +2692,7 @@ void spice_main_update_display(SpiceMainChannel
> *channel, int id,
> * Since: 0.35
> **/
> void spice_main_channel_update_display(SpiceMainChannel *channel, int id,
> int x, int y, int width,
> - int height, gboolean update)
> + int height, gboolean update)
> {
> SpiceMainChannelPrivate *c;
>
> ___
> Spice-commits mailing list
> spice-comm...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-commits
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/2] Add copr builds integration
Hi On Tue, Aug 27, 2019 at 6:27 PM Snir Sheriber wrote: > > This will let copr to generate srpm using the .copr/Makefile script Do we really want to maintain that kind of distro things upstream? Do we need it? What does it bring that gitlab CI doesn't have? > > Signed-off-by: Snir Sheriber > --- > .copr/Makefile | 29 + > 1 file changed, 29 insertions(+) > create mode 100644 .copr/Makefile > > diff --git a/.copr/Makefile b/.copr/Makefile > new file mode 100644 > index 000..db297fb > --- /dev/null > +++ b/.copr/Makefile > @@ -0,0 +1,29 @@ > +# This Makefile script is invoked by copr to build source rpm > +# See: https://docs.pagure.org/copr.copr/user_documentation.html#make-srpm When is the build actually triggered? Is copr monitoring the git repository? Is there a hook somewhere in gitlab to trigger the build? > + > +PROTOCOL_GIT_REPO = https://gitlab.freedesktop.org/spice/spice-protocol > +BUILD = meson gcc xz git rpm-build > + > +srpm: > + dnf install -y $(BUILD) > + > + # get upstream spice protocol > + git clone $(PROTOCOL_GIT_REPO) > + cd spice-protocol && meson -Dprefix=/usr/ build && ninja -C build > install > + rm -rf spice-protocol > + > + # get other dependencies for project excluding spice-protocol > + dnf install -y `sed '/^BuildRequires:/!d; s/.*://; > s/\bspice-protocol\b//; s/>.*//' *.spec.in` > + > + # do not use commit id for version > + git fetch --tags > + git describe --abbrev=0 | sed 's/v//' > .tarball-version > + # create source rpm > + meson --buildtype=release build > + # Meson does not update submodules recursively > + git submodule update --init --recursive > + # this fix an issue with Meson dist > + if ! test -r ../spice-common.git; then DIR=`basename "$$PWD"`; ln -s > "$$DIR/.git/modules/spice-common" ../spice-common.git; fi > + rm -rf meson-dist > + ninja -C build dist > + rpmbuild -bs ./build/*spec --define "_sourcedir > $$PWD/build/meson-dist/" --define "_srcrpmdir $(outdir)" Too much hacks for my taste here. > -- > 2.21.0 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] meson: Workaround gtk+ exposing Objective C in headers on macOS
On Wed, May 29, 2019 at 12:05 PM Frediano Ziglio wrote: > > Apply the same workaround in 3c9b37bfc7c88969dfe16b8bfd874745e0fceb8a > for Meson. > > Signed-off-by: Frediano Ziglio ack > --- > meson.build | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/meson.build b/meson.build > index 8c6288f3..4c065a43 100644 > --- a/meson.build > +++ b/meson.build > @@ -341,6 +341,11 @@ if spice_gtk_has_gtk > > '-DGDK_VERSION_MAX_ALLOWED=@0@'.format(gtk_encoded_version)] > endif > > +# Workaround gtk+ exposing Objective C: > https://gitlab.gnome.org/GNOME/gtk/issues/1737 > +if host_machine.system() == 'darwin' > + spice_gtk_global_cflags += ['-ObjC'] > +endif > + > > add_project_arguments(compiler.get_supported_arguments(spice_gtk_global_cflags), >language : 'c') > > -- > 2.20.1 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 spice-gtk] Adjust to window scaling
T(display));
> -wh = gtk_widget_get_allocated_height(GTK_WIDGET(display));
> +#if HAVE_EGL
> +if (egl_enabled(d)) {
> +gdk_scale = gtk_widget_get_scale_factor(GTK_WIDGET(display));
> +}
> +#endif
> +ww = gtk_widget_get_allocated_width(GTK_WIDGET(display)) * gdk_scale;
> +wh = gtk_widget_get_allocated_height(GTK_WIDGET(display)) *
> gdk_scale;
> } else {
> ww = fbw;
> wh = fbh;
> @@ -3091,7 +3105,8 @@ void spice_display_widget_gl_scanout(SpiceDisplay
> *display)
> g_clear_error();
> }
>
> -spice_egl_resize_display(display, d->ww, d->wh);
> +gint gdk_scale = gtk_widget_get_scale_factor(GTK_WIDGET(display));
> +spice_egl_resize_display(display, d->ww * gdk_scale, d->wh *
> gdk_scale);
> }
> #endif
>
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 00/13] Miscellaneous series
Hi
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> Hi,
>
> this series contains mostly fixes of some things
> that I came across while reading the spice-webdavd.c code,
> without any order.
>
> Cheers,
>
> Jakub Janků (13):
> spice: remove G_SOURCE_{REMOVE,CONTINUE} definitions
> spice: rename my_input_channel
> spice: handle SIGINT properly
> spice: quit service quickly
> spice: clear loop pointer on unref
> spice: print err when g_open fails
> spice: unref GSocketAddress
> spice-win: define SERVICE_NAME
> spice-win: remove MapDriveEnum
> spice-win: don't assign drive_letter on error
> spice: move OutputQueue to file
> rename README to README.md
> rename NEWS to NEWS.md
Looks good overall,
Phodav is hosted on GNOME gitlab, please open a MR there, so CI can run.
Thanks
> NEWS => NEWS.md | 0
> README => README.md | 21 ++--
> spice/meson.build | 8 +-
> spice/output-queue.c | 164 +
> spice/output-queue.h | 38 ++
> spice/spice-webdavd.c | 280 ++
> 6 files changed, 286 insertions(+), 225 deletions(-)
> rename NEWS => NEWS.md (100%)
> rename README => README.md (56%)
> create mode 100644 spice/output-queue.c
> create mode 100644 spice/output-queue.h
>
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 13/13] rename NEWS to NEWS.md
On Thu, May 23, 2019 at 10:38 AM Jakub Janků wrote: > > The filename needs the proper extension for GitLab to handle > the file as a Markdown file. > > Signed-off-by: Jakub Janků small nack, I am not sure how GNOME ftprelease will handle that change, at the moment they document NEWS file only: https://wiki.gnome.org/MaintainersCorner/Releasing > --- > NEWS => NEWS.md | 0 > 1 file changed, 0 insertions(+), 0 deletions(-) > rename NEWS => NEWS.md (100%) > > diff --git a/NEWS b/NEWS.md > similarity index 100% > rename from NEWS > rename to NEWS.md > -- > 2.21.0 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 12/13] rename README to README.md
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote: > > The filename needs the proper extension for GitLab to handle > the file as a Markdown file. > > Fix some style issues so that the file renders correctly. > > Signed-off-by: Jakub Janků why not, ack > --- > README => README.md | 21 ++--- > 1 file changed, 10 insertions(+), 11 deletions(-) > rename README => README.md (56%) > > diff --git a/README b/README.md > similarity index 56% > rename from README > rename to README.md > index fa82acd..297756f 100644 > --- a/README > +++ b/README.md > @@ -1,31 +1,30 @@ > -phởdav > -== > +# phởdav > > -= About > +## About > > phởdav is a WebDav server implementation using libsoup (RFC 4918). > > -Releases: http://ftp.gnome.org/pub/GNOME/sources/phodav/ > +Releases: http://ftp.gnome.org/pub/GNOME/sources/phodav/ > > -= Tools > +## Tools > > -chezdav, allows to share a particual directory (with optional > -digest authentication) > +* chezdav, allows to share a particual directory (with optional > + digest authentication) > > -= Notes > +## Notes > > phởdav was initially developped as a filesharing mechanism for Spice, > but it is generic enough to be reused in other projects, in particular > in the GNOME desktop. Further integration work would be a welcome > contribution! > > -= Contributing > +## Contributing > > git clone git://git.gnome.org/phodav > > -http://lists.freedesktop.org/mailman/listinfo/spice-devel > +http://lists.freedesktop.org/mailman/listinfo/spice-devel > > Please report bug there: > https://bugzilla.gnome.org/enter_bug.cgi?product=phodav > > -Forking and sending github pull requests is also welcome. > \ No newline at end of file > +Forking and sending pull requests is also welcome. > \ No newline at end of file > -- > 2.21.0 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 11/13] spice: move OutputQueue to file
gt;flushing);
> - g_warn_if_fail (!queue->idle_id);
> -
> - g_queue_free_full (queue->queue, g_free);
> - g_clear_object (>output);
> - g_free (queue);
> -}
> -
> -static OutputQueue*
> -output_queue_ref (OutputQueue *q)
> -{
> - q->refs++;
> - return q;
> -}
> -
> -static void
> -output_queue_unref (OutputQueue *q)
> -{
> - g_return_if_fail (q != NULL);
> -
> - q->refs--;
> - if (q->refs == 0)
> -output_queue_free (q);
> -}
> -
> -static gboolean output_queue_idle (gpointer user_data);
> -
> -static void
> -output_queue_flush_cb (GObject *source_object,
> - GAsyncResult *res,
> - gpointer user_data)
> -{
> - GError *error = NULL;
> - OutputQueueElem *e = user_data;
> - OutputQueue *q = e->queue;
> -
> - g_debug ("flushed");
> - q->flushing = FALSE;
> - g_output_stream_flush_finish (G_OUTPUT_STREAM (source_object),
> -res, );
> - if (error)
> -g_warning ("error: %s", error->message);
> -
> - g_clear_error ();
> -
> - if (!q->idle_id)
> -q->idle_id = g_idle_add (output_queue_idle, output_queue_ref (q));
> -
> - g_free (e);
> - output_queue_unref (q);
> -}
> -
> -static gboolean
> -output_queue_idle (gpointer user_data)
> -{
> - OutputQueue *q = user_data;
> - OutputQueueElem *e = NULL;
> - GError *error = NULL;
> -
> - if (q->flushing)
> -{
> - g_debug ("already flushing");
> - goto end;
> -}
> -
> - e = g_queue_pop_head (q->queue);
> - if (!e)
> -{
> - g_debug ("No more data to flush");
> - goto end;
> -}
> -
> - g_debug ("flushing %" G_GSIZE_FORMAT, e->size);
> - g_output_stream_write_all (q->output, e->buf, e->size, NULL, cancel,
> );
> - if (e->cb)
> -e->cb (q, e->user_data, error);
> -
> - if (error)
> - goto end;
> -
> - q->flushing = TRUE;
> - g_output_stream_flush_async (q->output, G_PRIORITY_DEFAULT, cancel,
> output_queue_flush_cb, e);
> -
> - q->idle_id = 0;
> - return FALSE;
> -
> -end:
> - g_clear_error ();
> - q->idle_id = 0;
> - g_free (e);
> - output_queue_unref (q);
> -
> - return FALSE;
> -}
> -
> -static void
> -output_queue_push (OutputQueue *q, const guint8 *buf, gsize size,
> - PushedCb pushed_cb, gpointer user_data)
> -{
> - OutputQueueElem *e;
> -
> - g_return_if_fail (q != NULL);
> -
> - e = g_new (OutputQueueElem, 1);
> - e->buf = buf;
> - e->size = size;
> - e->cb = pushed_cb;
> - e->user_data = user_data;
> - e->queue = q;
> - g_queue_push_tail (q->queue, e);
> -
> - if (!q->idle_id && !q->flushing)
> -q->idle_id = g_idle_add (output_queue_idle, output_queue_ref (q));
> -}
> -
> -
> static struct _DemuxData
> {
>gint64 client;
> @@ -264,7 +113,7 @@ add_client (GSocketConnection *client_connection)
>client->client_connection = g_object_ref (client_connection);
>// TODO: check if usage of this idiom is portable, or if we need to check
> collisions
>client->id = GPOINTER_TO_INT (client_connection);
> - client->queue = output_queue_new (bostream);
> + client->queue = output_queue_new (bostream, cancel);
>g_object_unref (bostream);
>
>g_hash_table_insert (clients, >id, client);
> @@ -280,7 +129,7 @@ client_free (Client *c)
>
>g_io_stream_close (G_IO_STREAM (c->client_connection), NULL, NULL);
>g_object_unref (c->client_connection);
> - output_queue_unref (c->queue);
> + g_object_unref (c->queue);
>g_free (c);
> }
>
> @@ -732,7 +581,7 @@ open_mux_path (const char *path)
>mux_istream = G_INPUT_STREAM (g_win32_input_stream_new (port_handle,
> TRUE));
> #endif
>
> - mux_queue = output_queue_new (G_OUTPUT_STREAM (mux_ostream));
> + mux_queue = output_queue_new (G_OUTPUT_STREAM (mux_ostream), cancel);
> }
>
> #ifdef G_OS_WIN32
> @@ -1002,12 +851,11 @@ run_service (ServiceData *service_data)
>g_clear_object (_istream);
>g_clear_object (_ostream);
>
> - output_queue_unref (mux_queue);
> + g_clear_object (_queue);
>g_hash_table_unref (clients);
>
>g_socket_service_stop (socket_service);
>
> - mux_queue = NULL;
>g_clear_object ();
>
> #ifdef G_OS_WIN32
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 10/13] spice-win: don't assign drive_letter on error
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> drive_letter should be assigned only when the mapping was successful.
>
> Signed-off-by: Jakub Janků
ack
> spice/spice-webdavd.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index 29058e7..f2c7f07 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -910,14 +910,20 @@ map_drive_cb(GTask *task,
>break;
> }
>
> - if (map_drive (drive_letter) != ERROR_ALREADY_ASSIGNED)
> + ret = map_drive (drive_letter);
> + if (ret == ERROR_ALREADY_ASSIGNED)
> {
> - break;
> + /* try again with another letter */
> + continue;
> +}
> + if (ret != NO_ERROR)
> +{
> + drive_letter = 0;
> }
> + break;
>//TODO: After mapping, rename network drive from
> \\localhost@PORT\DavWWWRoot
>// to something like SPICE Shared Folder
> }
> -
>g_mutex_lock(_drive_data->service_data->mutex);
>map_drive_data->service_data->drive_letter = drive_letter;
>g_mutex_unlock(_drive_data->service_data->mutex);
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 09/13] spice-win: remove MapDriveEnum
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> The enum doesn't add much value, let's remove it.
>
> Signed-off-by: Jakub Janků
ack
> ---
> spice/spice-webdavd.c | 19 +++
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index c77c870..29058e7 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -738,12 +738,6 @@ open_mux_path (const char *path)
> #ifdef G_OS_WIN32
> #define MAX_SHARED_FOLDER_NAME_SIZE 64
> #define MAX_DRIVE_LETTER_SIZE 3
> -typedef enum _MapDriveEnum
> -{
> - MAP_DRIVE_OK,
> - MAP_DRIVE_TRY_AGAIN,
> - MAP_DRIVE_ERROR
> -} MapDriveEnum;
>
> typedef struct _MapDriveData
> {
> @@ -828,7 +822,7 @@ netresource_free(NETRESOURCE *net_resource)
>g_free(net_resource->lpRemoteName);
> }
>
> -static MapDriveEnum
> +static guint32
> map_drive(const gchar drive_letter)
> {
>NETRESOURCE net_resource;
> @@ -841,16 +835,17 @@ map_drive(const gchar drive_letter)
>if (errn == NO_ERROR)
> {
>g_debug ("Shared folder mapped to %c succesfully", drive_letter);
> - return MAP_DRIVE_OK;
> }
>else if (errn == ERROR_ALREADY_ASSIGNED)
> {
>g_debug ("Drive letter %c is already assigned", drive_letter);
> - return MAP_DRIVE_TRY_AGAIN;
> +}
> + else
> +{
> + g_warning ("map_drive error %d", errn);
> }
>
> - g_warning ("map_drive error %d", errn);
> - return MAP_DRIVE_ERROR;
> + return errn;
> }
>
> static void
> @@ -915,7 +910,7 @@ map_drive_cb(GTask *task,
>break;
> }
>
> - if (map_drive (drive_letter) != MAP_DRIVE_TRY_AGAIN)
> + if (map_drive (drive_letter) != ERROR_ALREADY_ASSIGNED)
> {
> break;
> }
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 08/13] spice-win: define SERVICE_NAME
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> Signed-off-by: Jakub Janků
ack
> ---
> spice/spice-webdavd.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index 681e909..c77c870 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#define SERVICE_NAME "spice-webdavd"
> #endif
>
> #ifdef WITH_AVAHI
> @@ -1062,7 +1063,7 @@ service_main (DWORD argc, TCHAR *argv[])
>g_mutex_init(_data.mutex);
>
>service_status_handle =
> -RegisterServiceCtrlHandlerEx ("spice-webdavd", service_ctrl_handler,
> _data);
> +RegisterServiceCtrlHandlerEx (SERVICE_NAME, service_ctrl_handler,
> _data);
>
>g_return_if_fail (service_status_handle != 0);
>
> @@ -1154,7 +1155,7 @@ main (int argc, char *argv[])
>
>SERVICE_TABLE_ENTRY service_table[] =
> {
> - { (char *)"spice-webdavd", service_main }, { NULL, NULL }
> + { SERVICE_NAME, service_main }, { NULL, NULL }
> };
>if (!no_service && !getenv("DEBUG"))
> {
> --
> 2.21.0
>
> _______
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 07/13] spice: unref GSocketAddress
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> No need to keep it around after calling
> g_socket_listener_add_address().
>
> Signed-off-by: Jakub Janků
ack
> ---
> spice/spice-webdavd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index d9b1fae..681e909 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -1137,6 +1137,7 @@ main (int argc, char *argv[])
> NULL,
> NULL,
> );
> + g_object_unref (saddr);
>if (error)
> {
>g_printerr ("%s\n", error->message);
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 06/13] spice: print err when g_open fails
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> Don't fail silently when we cannot open the webdav virtio port.
>
> Signed-off-by: Jakub Janků
ack
> ---
> spice/spice-webdavd.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index 3fac28b..d9b1fae 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
> #endif
>
> #ifdef G_OS_WIN32
> @@ -703,8 +704,12 @@ open_mux_path (const char *path)
>g_debug ("Open %s", path);
> #ifdef G_OS_UNIX
>port_fd = g_open (path, O_RDWR);
> + gint errsv = errno;
>if (port_fd == -1)
> +{
> + g_printerr("Failed to open %s: %s\n", path, g_strerror(errsv));
>exit (1);
> +}
>
>wait_for_virtio_host (port_fd);
>
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 05/13] spice: clear loop pointer on unref
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote: > > The pointer to loop must be set to NULL on unref. > > Quit signal handler can be called at any time, > using g_main_loop_quit() on a freed loop could lead to segfault. I am not sure if this is enough to solve the race you describe (handler could still be called before loop is set to null, right?), but the change seems to go in the right direction. ack > > Signed-off-by: Jakub Janků > --- > spice/spice-webdavd.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c > index 6ad63c5..3fac28b 100644 > --- a/spice/spice-webdavd.c > +++ b/spice/spice-webdavd.c > @@ -243,7 +243,8 @@ quit (int sig) >if (sig == SIGINT || sig == SIGTERM) >quit_service = TRUE; > > - g_main_loop_quit (loop); > + if (loop) > +g_main_loop_quit (loop); > } > > static Client * > @@ -982,7 +983,7 @@ run_service (ServiceData *service_data) > >start_mux_read (mux_istream); >g_main_loop_run (loop); > - g_main_loop_unref (loop); > + g_clear_pointer (, g_main_loop_unref); > > #ifdef G_OS_WIN32 >g_cancellable_cancel (map_drive_data.cancel_map); > -- > 2.21.0 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 04/13] spice: quit service quickly
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> We shouldn't wait for 1 second if the service is supposed to stop.
>
> Signed-off-by: Jakub Janků
ack
> ---
> spice/spice-webdavd.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index cdfa73d..6ad63c5 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -923,11 +923,15 @@ map_drive_cb(GTask *task,
>
> #endif
>
> -static void
> +/* returns FALSE if the service should quit */
> +static gboolean
> run_service (ServiceData *service_data)
> {
>g_debug ("Run service");
>
> + if (quit_service)
> +return FALSE;
> +
> #ifdef G_OS_WIN32
>MapDriveData map_drive_data;
>map_drive_data.cancel_map = g_cancellable_new ();
> @@ -1003,6 +1007,7 @@ run_service (ServiceData *service_data)
> #else
>close (port_fd);
> #endif
> + return !quit_service;
> }
>
> #ifdef G_OS_WIN32
> @@ -1064,9 +1069,8 @@ service_main (DWORD argc, TCHAR *argv[])
>service_status.dwWaitHint = 0;
>SetServiceStatus (service_status_handle, _status);
>
> - while (!quit_service) {
> - run_service (_data);
> - g_usleep (G_USEC_PER_SEC);
> + while (run_service(_data)) {
> +g_usleep(G_USEC_PER_SEC);
>}
>
>service_status.dwCurrentState = SERVICE_STOPPED;
> @@ -1154,8 +1158,7 @@ main (int argc, char *argv[])
> }
> } else
> #endif
> - while (!quit_service) {
> -run_service (_data);
> + while (run_service(_data)) {
> g_usleep (G_USEC_PER_SEC);
>}
>
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 03/13] spice: handle SIGINT properly
Hi
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> According to [0], g_debug should not be used in a signal handler.
> So, to avoid reentrancy, do not print debug message when quit is
> called with SIGINT.
>
> [0]
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2019
>
> Signed-off-by: Jakub Janků
> ---
> spice/spice-webdavd.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index e494692..cdfa73d 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -237,7 +237,8 @@ static void mdns_unregister_service (void);
> static void
> quit (int sig)
> {
> - g_debug ("quit %d", sig);
> + if (sig != SIGINT)
> +g_debug ("quit %d", sig);
>
I would simply remove the g_debug() call then.
(maybe we should have a different function for the signal handler)
>if (sig == SIGINT || sig == SIGTERM)
>quit_service = TRUE;
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 02/13] spice: rename my_input_channel
{
> - my_input_stream_read_async (istream,
> - , sizeof (gint64),
> G_PRIORITY_DEFAULT,
> - cancel, mux_client_read_cb, NULL);
> + input_stream_read_async_thread (istream,
> + , sizeof (gint64),
> G_PRIORITY_DEFAULT,
> + cancel, mux_client_read_cb, NULL);
> }
>
> static void client_start_read (Client *client);
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH phodav 01/13] spice: remove G_SOURCE_{REMOVE, CONTINUE} definitions
On Thu, May 23, 2019 at 10:37 AM Jakub Janků wrote:
>
> Other symbols that were added after G_SOURCE_{REMOVE, CONTINUE}
> are already being used in the rest of the file - e.g. g_task_new,
> so there's no need to define these.
>
> Signed-off-by: Jakub Janků
ack,
we should bump glib dep to 2.36 at least (gtask)
> ---
> spice/spice-webdavd.c | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/spice/spice-webdavd.c b/spice/spice-webdavd.c
> index cd0029b..81fe6be 100644
> --- a/spice/spice-webdavd.c
> +++ b/spice/spice-webdavd.c
> @@ -665,13 +665,6 @@ mdns_state_changed (GaClient *client, GaClientState
> state, gpointer user_data)
> }
> #endif
>
> -#ifndef G_SOURCE_REMOVE
> -#define G_SOURCE_REMOVE FALSE
> -#endif
> -#ifndef G_SOURCE_CONTINUE
> -#define G_SOURCE_CONTINUE TRUE
> -#endif
> -
> #ifdef G_OS_UNIX
> static void
> wait_for_virtio_host (gint fd)
> --
> 2.21.0
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 spice-gtk] Adjust to window scaling
Hi
On Sun, Mar 17, 2019 at 4:28 PM Snir Sheriber wrote:
>
> When GDK_SCALE is != 1 and egl is used, the image presented does not
> fit to the window (scale of 2 is often used with hidpi monitors).
> Usually this is not a problem since all components are adjusted by
> gdk/gtk but with egl, pixel-based data is not being scaled. In this
> case window's scale value can be used in order to determine whether
> to use a pixel resource with higher resolution data.
>
> In order to reproduce the problem set spice with virgl/Intel-vGPU
> and run spice-gtk with GDK_SCALE=2
> ---
> Changes from v1:
> -commit msg
> -replace var naming (ws with win_scale)
>
>
> This patch is kind of RFC, it fixes the issue, but it's a bit hacky
> and specific. I didn't come across other scale issues but it is likely
> that more of these exist and better and generic fix is needed.
>
> ---
> src/spice-widget-egl.c | 15 +--
> src/spice-widget-priv.h | 1 +
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
> index 43fccd7..600c87a 100644
> --- a/src/spice-widget-egl.c
> +++ b/src/spice-widget-egl.c
> @@ -326,6 +326,8 @@ static gboolean spice_widget_init_egl_win(SpiceDisplay
> *display, GdkWindow *win,
> if (d->egl.surface)
> return TRUE;
>
> +d->egl.scale = gdk_window_get_scale_factor(win);
Why not use gtk_widget_get_scale_factor() directly from
spice_egl_resize_display?
> +
> #ifdef GDK_WINDOWING_X11
> if (GDK_IS_X11_WINDOW(win)) {
> native = (EGLNativeWindowType)GDK_WINDOW_XID(win);
> @@ -431,15 +433,17 @@ void spice_egl_resize_display(SpiceDisplay *display,
> int w, int h)
> {
> SpiceDisplayPrivate *d = display->priv;
> int prog;
> +gint win_scale;
>
> if (!gl_make_current(display, NULL))
> return;
>
> +win_scale = d->egl.scale;
> glGetIntegerv(GL_CURRENT_PROGRAM, );
>
> glUseProgram(d->egl.prog);
> -apply_ortho(d->egl.mproj, 0, w, 0, h, -1, 1);
> -glViewport(0, 0, w, h);
> +apply_ortho(d->egl.mproj, 0, w * win_scale , 0, h * win_scale, -1, 1);
> +glViewport(0, 0, w * win_scale, h * win_scale);
>
> if (d->ready)
> spice_egl_update_display(display);
> @@ -559,6 +563,13 @@ void spice_egl_update_display(SpiceDisplay *display)
>
> spice_display_get_scaling(display, , , , , );
>
> +// Adjust to gdk scale
> +s *= d->egl.scale;
> +x *= d->egl.scale;
> +y *= d->egl.scale;
> +w *= d->egl.scale;
> +h *= d->egl.scale;
> +
> glClearColor(0.0f, 0.0f, 0.0f, 0.0f);
> glClear(GL_COLOR_BUFFER_BIT);
>
> diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> index 65eb404..8f110ac 100644
> --- a/src/spice-widget-priv.h
> +++ b/src/spice-widget-priv.h
> @@ -149,6 +149,7 @@ struct _SpiceDisplayPrivate {
> EGLImageKHR image;
> gbooleancall_draw_done;
> SpiceGlScanout scanout;
> +gint scale;
> } egl;
> #endif // HAVE_EGL
> double scroll_delta_y;
> --
> 2.19.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH linux/vd-agent 10/11] clipboard: only send release when no immediate grab
Hi
On Fri, Mar 29, 2019 at 10:17 AM Frediano Ziglio wrote:
>
> >
> > Hi
> >
> > On Thu, Mar 28, 2019 at 5:30 PM Frediano Ziglio wrote:
> > >
> > > >
> > > > From: Marc-André Lureau
> > > >
> > > > Do not send a release event between two grabs, this helps with window
> > > > manager interaction issues on peer side.
> > > >
> > >
> > > I would explain which kind of issue this is supposed to fix.
> >
> > They react to clipboard events in different ways. The point is to
> > minimize the amount of events to avoid extra unnecessary interactions.
> >
> > In particular, on release event, some clipboard managers take the
> > grab. This creates a race with Spice which does a release between
> > grabs currently.
> >
>
> As clipboard managers are not using SPICE I suppose here you
> are talking about desktop clipboard release, not agent RELEASE
> message. But on the previous sentence we were speaking about
> agent RELEASE.
Yes, when a peer receives RELEASE, it will effectively release the
grab on its desktop, and this may make the desktop clipboard agent
react.
>
> > >
> > > > Advertise this behaviour via a capability introduced in spice-protocol
> > > > 0.12.16, so the client doesn't need to do some time-based filtering.
> > > >
> > > > (the capability shouldn't need to be negotiated, a client shouldn't
> > > > expect a release between two grabs)
> > > >
> > > > Signed-off-by: Marc-André Lureau
> > >
> > > I suppose Jakub/Victor could do a much better review/test than me.
> > >
> > > > ---
> > > > src/vdagent/clipboard.c | 12 ++--
> > > > src/vdagent/x11.c | 7 +++
> > > > src/vdagentd/vdagentd.c | 1 +
> > > > 3 files changed, 10 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> > > > index 9fb87e3..097c6ee 100644
> > > > --- a/src/vdagent/clipboard.c
> > > > +++ b/src/vdagent/clipboard.c
> > > > @@ -242,13 +242,13 @@ static void clipboard_owner_change_cb(GtkClipboard
> > > > *clipboard,
> > > > return;
> > > > }
> > > >
> > > > -if (sel->owner == OWNER_GUEST) {
> > > > -clipboard_new_owner(c, sel_id, OWNER_NONE);
> > > > -udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> > > > NULL,
> > > > 0);
> > > > -}
> > > > -
> > > > -if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER)
> > > > +if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > > > +if (sel->owner == OWNER_GUEST) {
> > > > +clipboard_new_owner(c, sel_id, OWNER_NONE);
> > > > +udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> > > > NULL, 0);
> > > > +}
> > > > return;
> > > > +}
> > > >
> > > > /* if there's a pending request for clipboard targets, cancel it */
> > > > if (sel->last_targets_req)
> > > > diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> > > > index c2515a8..9409b53 100644
> > > > --- a/src/vdagent/x11.c
> > > > +++ b/src/vdagent/x11.c
> > > > @@ -530,11 +530,10 @@ static void vdagent_x11_handle_event(struct
> > > > vdagent_x11
> > > > *x11, XEvent event)
> > > > if (ev.xfev.owner == x11->selection_window)
> > > > return;
> > > >
> > > > -/* If the clipboard owner is changed we no longer own it */
> > >
> > > Why not keeping this comment?
> >
> > The comment is no longer valid with this change: if the clipboard
> > owner is changed, the agent may still own the grab (at the protocol
> > level).
> >
>
> Yes, this prove what I was saying in the previous e-mail, you are
> mixing desktop ownership and SPICE grab, if you consider from
> desktop prospective (which is using owner definition like in
> "ev.xfev.owner") the comment is still valid, but you are reading
> it with different definitions so it became invalid.
>
> > As you can see with the following line being removed:
> > >
> > > > -vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> > > > -
> > > > -
Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL
he grab in the > > > > client desktop environment, but another client application. > > > > > > > > In general, when talking about the protocol, "client has the grab" > > > > means 2) to me, iow it can provide data to the remote. > > > > > > > > > > I think I mean the opposite. The reason is that some application > > > have the ownership on either side, when you send a VD_AGENT_CLIPBOARD_GRAB > > > the current local application (so spice-gtk/vd_agent) does NOT have the > > > ownership and it's asking the remote to take to the application (so will > > > remove the ownership from another remote application). > > > From how I read the message (VD_AGENT_CLIPBOARD_GRAB) is telling the > > > other side to grab (take ownership) of the clipboard so will be the > > > remote to have the "grab", not the local. > > > > > > I find it hard to understand you. The sequence of events is as such. > > > > A & B are client or remote exchangeably: > > - an application on A takes the clipboard grab (== announce clipboard > > data is available) > > - A get notified by the desktop and sends a GRAB message to B side > > - B receives GRAB, and takes the clipboard grab on B desktop side > > > > With this sequence, "A" has the clipboard grab at the Spice protocol > > level, so to say. It means there is clipboard data on A side. > > > > Not clear either. If what you wrote it's correct, what I read: > You wrote "B receives GRAB, and takes the clipboard grab" so I suppose > that B now have the "clipboard grab", however you also wrote > "A has the clipboard grab at the Spice protocol level" so if this is true > here "clipboard grab" and "clipboard grab at the Spice protocol level" are > two distinct definitions. To me looks like you are overloading some I said "B receives GRAB, and takes the clipboard grab on B desktop side": on desktop side. A has the grab at the protocol level on behalf of A desktop grab. > definition here. In particular mixing the concept of clipboard ownership > from the local desktop prospective (all GTK, X11 and Windows have this > concept of clipboard owner) with SPICE one, which is why I'm confused. > They are related indeed but are not the same thing. These patches > are actually changing the relation between them so confusing the > two definition does not help. I am not confused, you are :) > > > > > > > > > > Not sure what is the initial state, when you connect. > > > > > > > > Initial state is undefined, and currently whatever the guest or client > > > > side state is. Iow, Spice client/agent doen't do anything afaik. They > > > > will only react to further grab events. > > > > > > > > > > I suppose the agent will keep its state while the client if was not > > > connected get some initial state (boolean variables have to be true or > > > false at the end). > > > > > > > We could change the client or the guest to take the grab on connect, > > > > if clipboard data is available. That doesn't require protocol change. > > > > Although in case of conflict, we would probably end in the same > > > > situation that this protocol change is aiming to solve. > > > > > > > > > > When there's a disconnection we should drop the ownership as we > > > cannot anymore get the data from the remote in case other application > > > are asking so the "disconnected" state should be no ownership of > > > the clipboard on both ends (although I suppose the client will be > > > closed at that time). > > > > That's an implementation issue, not related to the protocol and the > > problem discussed here. > > > > > > > > Once a connection happens we could however have both ends with > > > data on the clipboard (the total states are 3 (a) client/agent have > > > ownership (b) other application have ownership (c) no ownership/data > > > on clipboard, here (a) should be impossible). > > > Which one "win" I would say hard to tell. > > > > > > > If both sides would try to take the grab simultaneously at init time, > > they would race, and we get back to the problem we are solving here. > > -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL
Hi
On Thu, Mar 28, 2019 at 6:05 PM Frediano Ziglio wrote:
>
> >
> > ..Hi
> >
> > On Thu, Mar 28, 2019 at 4:14 PM Frediano Ziglio wrote:
> > > > The role of the grab message is to take ownership of the clipboard (to
> > > > advertize clipboard data available). It may come at any time from both
> > > > side, and override the current grab owner. It may come from the guest
> > > > (after C-c in guest app, for ex), so the client grabs the clipboard.
> > > > Or it may come from the client (C-c in client side app), and the agent
> > > > will grab the guest clipboard.
> > > >
> > >
> > > Yes, I realized this morning thinking about how clipboards works
> > > (very rusty in my mind).
> > > Instead of setting it you get the ownership that is you are willing
> > > to set a value on the clipboard but you defer setting it to avoid
> > > the expense of data copy.
> > > So, the old (original?) protocol was simply to sending entire data
> > > on every change, this avoided to loose the clipboard data entirely.
> >
> > I don't even remember that version. It looks like the original version
> > was already "on-demand"
> >
> >
> > commit 5fdd0ba6650919dcfd7740c041ad7d2b9c4560ab
> > Author: Arnon Gilboa
> > Date: Mon Oct 4 16:45:15 2010 +0200
> >
> > vd_agent: add protocol messages for clipboard/selection-owner model
> >
> > -VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an
> > application in our side ("we") got ownership of the clipboard.
> > -VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side
> > owns the clipboard (GRAB received), we notify the os we are the owner.
> > when we are asked by the os/app for the clipboard data, we send this
> > RE
> > QUEST msg to the other side.
> > -VD_AGENT_CLIPBOARD(type, data) - the existing message for sending
> > the clipboard, is now sent only in response to REQUEST.
> > -VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no
> > longer the owner of the clipboard (e.g. the owner app was closed).
> >
> > this patch will be followed by agent & client patches handling the
> > above messages.
> >
> >
> >
> > So VD_AGENT_CAP_CLIPBOARD_BY_DEMAND simply means clipboard support to me.
> >
>
> I suppose the "now" in "the existing message for sending the clipboard, is
> now sent only in response to REQUEST" means that was changed.
>
> I personally think the code should be readable from a tarball/snapshot not
> pretending people to dig into 10 years of history. I'll try to find some time
> to put these lines into vd_agent.h.
>
> > > The current is: if we get new local clipboard data send the "grab"
> > > so remote knows that will have to read our data.
> >
> > yes
> >
> > > So either we have ownership/grab, meaning the data are remote
> > > waiting to get fetched or we don't have ownership and the remote
> > > should be grabbing as we have data to send (at least in a stable
> > > state).
> >
> >
> > That last sentence is confusing. There are 2 states the client can
> > "have the grab".
> >
> > 1. the client took the grab for the remote data: we are talking about
> > the client app in the client desktop environment
> > 2. the client took the grab, at the protocol level: it sent a grab
> > over the protocol to announce it can provide clipboard data to the
> > remote. In this case, the client app doesn't have the grab in the
> > client desktop environment, but another client application.
> >
> > In general, when talking about the protocol, "client has the grab"
> > means 2) to me, iow it can provide data to the remote.
> >
>
> I think I mean the opposite. The reason is that some application
> have the ownership on either side, when you send a VD_AGENT_CLIPBOARD_GRAB
> the current local application (so spice-gtk/vd_agent) does NOT have the
> ownership and it's asking the remote to take to the application (so will
> remove the ownership from another remote application).
> From how I read the message (VD_AGENT_CLIPBOARD_GRAB) is telling the
> other side to grab (take ownership) of the clipboard so will be the
> remote to have the "grab", not the local.
I find it hard to understand you. The sequence of events is as such.
A & B are client or remote exchangeably:
- an application on A takes the clipboard grab (== announce clipboard
data is available)
- A get notified by the desktop and sends a GRAB message to B side
- B receives GRAB, and takes the clipboard grab on B desktop side
With this sequence, "A" has the clipboard grab at the Spice protocol
level, so to say. It means there is clipboard data on A side.
> >
> > > Not sure what is the initial state, when you connect.
> >
> > Initial state is undefined, and currently whatever the guest or client
> > side state is. Iow, Spice client/agent doen't do anything afaik. They
> > will only react to further grab events.
> >
>
> I suppose the agent will keep its state while the client if was not
> connected get some initial state (boolean variables have to be true or
> false at the end).
>
> > We
Re: [Spice-devel] [PATCH linux/vd-agent 10/11] clipboard: only send release when no immediate grab
Hi
On Thu, Mar 28, 2019 at 5:30 PM Frediano Ziglio wrote:
>
> >
> > From: Marc-André Lureau
> >
> > Do not send a release event between two grabs, this helps with window
> > manager interaction issues on peer side.
> >
>
> I would explain which kind of issue this is supposed to fix.
They react to clipboard events in different ways. The point is to
minimize the amount of events to avoid extra unnecessary interactions.
In particular, on release event, some clipboard managers take the
grab. This creates a race with Spice which does a release between
grabs currently.
>
> > Advertise this behaviour via a capability introduced in spice-protocol
> > 0.12.16, so the client doesn't need to do some time-based filtering.
> >
> > (the capability shouldn't need to be negotiated, a client shouldn't
> > expect a release between two grabs)
> >
> > Signed-off-by: Marc-André Lureau
>
> I suppose Jakub/Victor could do a much better review/test than me.
>
> > ---
> > src/vdagent/clipboard.c | 12 ++--
> > src/vdagent/x11.c | 7 +++
> > src/vdagentd/vdagentd.c | 1 +
> > 3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> > index 9fb87e3..097c6ee 100644
> > --- a/src/vdagent/clipboard.c
> > +++ b/src/vdagent/clipboard.c
> > @@ -242,13 +242,13 @@ static void clipboard_owner_change_cb(GtkClipboard
> > *clipboard,
> > return;
> > }
> >
> > -if (sel->owner == OWNER_GUEST) {
> > -clipboard_new_owner(c, sel_id, OWNER_NONE);
> > -udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0, NULL,
> > 0);
> > -}
> > -
> > -if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER)
> > +if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > +if (sel->owner == OWNER_GUEST) {
> > +clipboard_new_owner(c, sel_id, OWNER_NONE);
> > +udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> > NULL, 0);
> > +}
> > return;
> > +}
> >
> > /* if there's a pending request for clipboard targets, cancel it */
> > if (sel->last_targets_req)
> > diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> > index c2515a8..9409b53 100644
> > --- a/src/vdagent/x11.c
> > +++ b/src/vdagent/x11.c
> > @@ -530,11 +530,10 @@ static void vdagent_x11_handle_event(struct
> > vdagent_x11
> > *x11, XEvent event)
> > if (ev.xfev.owner == x11->selection_window)
> > return;
> >
> > -/* If the clipboard owner is changed we no longer own it */
>
> Why not keeping this comment?
The comment is no longer valid with this change: if the clipboard
owner is changed, the agent may still own the grab (at the protocol
level).
As you can see with the following line being removed:
>
> > -vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> > -
> > -if (ev.xfev.owner == None)
> > +if (ev.xfev.owner == None) {
> > +vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> > return;
> > +}
> >
> > /* Request the supported targets from the new owner */
> > XConvertSelection(x11->display, ev.xfev.selection,
> > x11->targets_atom,
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index 72a3e13..683e5d3 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -133,6 +133,7 @@ static void send_capabilities(struct vdagent_virtio_port
> > *vport,
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GRAPHICS_DEVICE_INFO);
> > +VD_AGENT_SET_CAPABILITY(caps->caps,
> > VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB);
> > virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
> >
> > vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH linux/vd-agent 06/11] configure: bump gobject >= 2.50
Hi
On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku wrote:
>
> Hi,
>
> On Fri, Mar 22, 2019 at 4:13 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > This is required for using the new GObject macros.
>
> Which macros are you referring to?
> G_DECLARE_FINAL_TYPE is available since 2.44
That's right, we could probably take 2.44. But since our min os
requirement is 2.50 already, it's easier to directly bump there.
> >
> > According to commit 61fc548fe1a323dd2344c8ae267e3ce05e86da7d ("Bump
> > GLib version to 2.34"), RHEL6 is no longer supported.
> >
> > GLib version across some distributions, from repology:
> > - Debian Stable (9): 2.50.3
> > - CentOS 7: 2.56.1
> > - Fedora 26: 2.52.3 (fwiw, Fedora 30: 2.60.0)
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > configure.ac | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index fb0675f..9808f58 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -100,7 +100,7 @@ AC_ARG_ENABLE([static-uinput],
> >[enable_static_uinput="$enableval"],
> >[enable_static_uinput="no"])
> >
> > -PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.34])
> > +PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.50])
>
> You could bump glib version as well
5/11 replaced glib by gobject, so it's implicit now.
>
> > PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
> > PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.16])
> > PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
> > --
> > 2.21.0.4.g36eb1cb9cf
> >
>
> Cheers,
> Jakub
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH linux/vd-agent 06/11] configure: bump gobject >= 2.50
Hi
On Thu, Mar 28, 2019 at 4:27 PM Frediano Ziglio wrote:
>
> >
> > From: Marc-André Lureau
> >
> > This is required for using the new GObject macros.
> >
> > According to commit 61fc548fe1a323dd2344c8ae267e3ce05e86da7d ("Bump
> > GLib version to 2.34"), RHEL6 is no longer supported.
> >
> > GLib version across some distributions, from repology:
> > - Debian Stable (9): 2.50.3
> > - CentOS 7: 2.56.1
> > - Fedora 26: 2.52.3 (fwiw, Fedora 30: 2.60.0)
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > configure.ac | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index fb0675f..9808f58 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -100,7 +100,7 @@ AC_ARG_ENABLE([static-uinput],
> >[enable_static_uinput="$enableval"],
> >[enable_static_uinput="no"])
> >
> > -PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.34])
> > +PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.50])
> > PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
> > PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.16])
> > PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
>
> I don't understand why not directly do it in 5/11, or why
> not also bumping glib-2.0 to 2.50, are they not directly
> related?
5/11 switches from glib dep to gobject dep.
6/11 bump version requirement.
The 2 should probably be done & discussed separately.
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL
..Hi
On Thu, Mar 28, 2019 at 4:14 PM Frediano Ziglio wrote:
> > The role of the grab message is to take ownership of the clipboard (to
> > advertize clipboard data available). It may come at any time from both
> > side, and override the current grab owner. It may come from the guest
> > (after C-c in guest app, for ex), so the client grabs the clipboard.
> > Or it may come from the client (C-c in client side app), and the agent
> > will grab the guest clipboard.
> >
>
> Yes, I realized this morning thinking about how clipboards works
> (very rusty in my mind).
> Instead of setting it you get the ownership that is you are willing
> to set a value on the clipboard but you defer setting it to avoid
> the expense of data copy.
> So, the old (original?) protocol was simply to sending entire data
> on every change, this avoided to loose the clipboard data entirely.
I don't even remember that version. It looks like the original version
was already "on-demand"
commit 5fdd0ba6650919dcfd7740c041ad7d2b9c4560ab
Author: Arnon Gilboa
Date: Mon Oct 4 16:45:15 2010 +0200
vd_agent: add protocol messages for clipboard/selection-owner model
-VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an
application in our side ("we") got ownership of the clipboard.
-VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side
owns the clipboard (GRAB received), we notify the os we are the owner.
when we are asked by the os/app for the clipboard data, we send this
RE
QUEST msg to the other side.
-VD_AGENT_CLIPBOARD(type, data) - the existing message for sending
the clipboard, is now sent only in response to REQUEST.
-VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no
longer the owner of the clipboard (e.g. the owner app was closed).
this patch will be followed by agent & client patches handling the
above messages.
So VD_AGENT_CAP_CLIPBOARD_BY_DEMAND simply means clipboard support to me.
> The current is: if we get new local clipboard data send the "grab"
> so remote knows that will have to read our data.
yes
> So either we have ownership/grab, meaning the data are remote
> waiting to get fetched or we don't have ownership and the remote
> should be grabbing as we have data to send (at least in a stable
> state).
That last sentence is confusing. There are 2 states the client can
"have the grab".
1. the client took the grab for the remote data: we are talking about
the client app in the client desktop environment
2. the client took the grab, at the protocol level: it sent a grab
over the protocol to announce it can provide clipboard data to the
remote. In this case, the client app doesn't have the grab in the
client desktop environment, but another client application.
In general, when talking about the protocol, "client has the grab"
means 2) to me, iow it can provide data to the remote.
> Not sure what is the initial state, when you connect.
Initial state is undefined, and currently whatever the guest or client
side state is. Iow, Spice client/agent doen't do anything afaik. They
will only react to further grab events.
We could change the client or the guest to take the grab on connect,
if clipboard data is available. That doesn't require protocol change.
Although in case of conflict, we would probably end in the same
situation that this protocol change is aiming to solve.
> But from the stable state (one and only one has the ownership)
> is not clear how you get both sending the grab message at the same
> time (the one without ownership should not send the grab).
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL
Hi
On Wed, Mar 27, 2019 at 4:50 PM Frediano Ziglio wrote:
>
> > Hi
> >
> > On Wed, Mar 27, 2019 at 8:23 AM Frediano Ziglio wrote:
> > >
> > > >
> > > > From: Marc-André Lureau
> > > >
> > > > When this capability is negoticated by both the client & the agent,
> > >
> > > negotiated
> > >
> > > > the clipboard grab messages have an associated serial counter.
> > > >
> > > > The serial is reset to 0 upon client connection.
> > > >
> > > > The counter is increment by 1 on each grab message, by both sides.
> > > >
> > >
> > > What would happen in case of restart of the guest? How the guest is
> > > supposed to keep the old serial?
> >
> > This is like a new client-agent connection in this case, it starts again
> > from
> > 0.
> >
> > > I think you can achieve sending the serial with an additional separate
> > > message at the beginning.
> >
> > I don't think it's necessary, but I am may be missing something.
> >
>
> Well, adding a field or a message are both changes, just different.
>
> > > I don't think this new protocol is designed to support multiple
> > > clients.
> >
> > clipboard sharing isn't designed for multiple client either.
> >
> > >
> > > > The sender of the message with the highest serial should be the
> > > > clipboard grab owner, and the current session serial should be
> > > > updated.
> > > >
> > > > If a lower serial than the current session serial is received, the
> > > > grab should be discarded.
> > > >
> > > > Whenever two grabs share the same serial, the one coming from the
> > > > client should have a higher priority and the client should gain the
> > > > clipboard ownership.
> > > >
> > > > No special treatement is done for the unlikely case of overflowing the
> > >
> > > treatment
> >
> > ok
> >
> > >
> > > > counter. It may temporarily inverse the priority, until both side have
> > > > overflown and/or synchronized.
> > > >
> > >
> > > synchronized? So there's a single counter for both guest and client?
> > > I though were two counters, one for each side.
> >
> > Conceptually, it's the "same" counter.
> >
>
> Okay, it was not clear.
>
> > >
> > > > Note: this mechanism isn't aiming at making "the most recent" (as in
> > > > time) side gaining the ownership. One side sending subsequent grab
> > > > messages earlier will likely take the ownership over a side sending a
> > > > single message simultaneously the other way. It only clears the
> > > > situation where both side believe that the other is the current
> > > > clipboard owner, by having a global ordering and priority in case of
> > > > serial conflict.
> > > >
> > > > Signed-off-by: Marc-André Lureau
> > > > ---
> > > > spice/vd_agent.h | 4
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > > index 862cb5c..9ae3cc7 100644
> > > > --- a/spice/vd_agent.h
> > > > +++ b/spice/vd_agent.h
> > > > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED
> > > > VDAgentClipboardGrab
> > > > {
> > > > #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > > > uint8_t selection;
> > > > uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > > > +#endif
> > > > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > > > +uint32_t serial;
> > > > #endif
> > >
> > > I would prefer a new message instead of partial structures.
> >
> > VDAgentClipboardGrabSelectionAndSerialAnd... ?
> >
>
> Usually a version schema is used like VDAgentClipboardGrabSelection_v2
>
> > yeah, it's not ideal. I wish we would use a better protocol, be it
> > json or protobuf etc..
> >
>
> This is not a justification to produce even worse code.
> A plain C structure would be fine, many binary protocols (like IP)
> uses this method.
> A semi-defined C structure with missing fields is the worst I
> ever seen, continuing to use is not so sensible.
Different trade-offs, version vs capabilities.
Since we don't have missing fields in the protocol, this
Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL
Hi
On Wed, Mar 27, 2019 at 8:23 AM Frediano Ziglio wrote:
>
> >
> > From: Marc-André Lureau
> >
> > When this capability is negoticated by both the client & the agent,
>
> negotiated
>
> > the clipboard grab messages have an associated serial counter.
> >
> > The serial is reset to 0 upon client connection.
> >
> > The counter is increment by 1 on each grab message, by both sides.
> >
>
> What would happen in case of restart of the guest? How the guest is
> supposed to keep the old serial?
This is like a new client-agent connection in this case, it starts again from 0.
> I think you can achieve sending the serial with an additional separate
> message at the beginning.
I don't think it's necessary, but I am may be missing something.
> I don't think this new protocol is designed to support multiple
> clients.
clipboard sharing isn't designed for multiple client either.
>
> > The sender of the message with the highest serial should be the
> > clipboard grab owner, and the current session serial should be
> > updated.
> >
> > If a lower serial than the current session serial is received, the
> > grab should be discarded.
> >
> > Whenever two grabs share the same serial, the one coming from the
> > client should have a higher priority and the client should gain the
> > clipboard ownership.
> >
> > No special treatement is done for the unlikely case of overflowing the
>
> treatment
ok
>
> > counter. It may temporarily inverse the priority, until both side have
> > overflown and/or synchronized.
> >
>
> synchronized? So there's a single counter for both guest and client?
> I though were two counters, one for each side.
Conceptually, it's the "same" counter.
>
> > Note: this mechanism isn't aiming at making "the most recent" (as in
> > time) side gaining the ownership. One side sending subsequent grab
> > messages earlier will likely take the ownership over a side sending a
> > single message simultaneously the other way. It only clears the
> > situation where both side believe that the other is the current
> > clipboard owner, by having a global ordering and priority in case of
> > serial conflict.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > spice/vd_agent.h | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index 862cb5c..9ae3cc7 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab {
> > #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > uint8_t selection;
> > uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > +#endif
> > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > +uint32_t serial;
> > #endif
>
> I would prefer a new message instead of partial structures.
VDAgentClipboardGrabSelectionAndSerialAnd... ?
yeah, it's not ideal. I wish we would use a better protocol, be it
json or protobuf etc..
> Why not reusing part of __reserved?
Because it's only 24 bits there, and if we make it too small, we would
have to deal explicitly with rounding issues.
>
> > uint32_t types[0];
> > } VDAgentClipboardGrab;
> > @@ -288,6 +291,7 @@ enum {
> > VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
> > VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB,
> > +VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL,
> > VD_AGENT_END_CAP,
> > };
> >
>
> I lost the original issue. Won't be easier to just define a static precedence,
> like "in case of conflict the client win"? It would avoid to have 2 cases to
> test,
> each potentially with problems to solve.
You mean without this request serial?
How would you "legitimately steal" the client grab then?
thanks
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH linux/vd-agent 08/11] clipboard: gobject-ify VDAgentClipboards
Hi
On Mon, Mar 25, 2019 at 10:26 AM Frediano Ziglio wrote:
>
> >
> > From: Marc-André Lureau
> >
> > This will allow easier lifecycle management,
> > and usage of gtk_clipboard_set_with_owner()
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/vdagent/clipboard.c | 67 +++--
> > src/vdagent/clipboard.h | 12 +---
> > src/vdagent/vdagent.c | 7 +++--
> > 3 files changed, 56 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> > index 1e49248..cf6e344 100644
> > --- a/src/vdagent/clipboard.c
> > +++ b/src/vdagent/clipboard.c
> > @@ -76,15 +76,25 @@ typedef struct {
> > } Selection;
> > #endif
> >
> > -struct VDAgentClipboards {
> > -#ifdef WITH_GTK
> > +struct _VDAgentClipboards {
>
> Can we use C99 complaints identifiers?
I didn't think much about the struct identifiers.
fwiw, glib/gobject/gtk uses "struct _Foo" everywhere.
>
> > +GObject parent;
> > +
> > struct udscs_connection *conn;
> > -Selectionselections[SELECTION_COUNT];
> > +
> > +#ifdef WITH_GTK
> > +Selection selections[SELECTION_COUNT];
> > #else
> > struct vdagent_x11 *x11;
> > #endif
> > };
> >
> > +struct _VDAgentClipboardsClass
> > +{
>
> 2 structure style declaration in one patch
Hmm? are you talking about braces indentation here?
>
> > +GObjectClass parent;
> > +};
> > +
> > +G_DEFINE_TYPE(VDAgentClipboards, vdagent_clipboards, G_TYPE_OBJECT)
> > +
> > #ifdef WITH_GTK
> > static const struct {
> > guint type;
> > @@ -453,43 +463,56 @@ err:
> > #endif
> > }
> >
> > -VDAgentClipboards *vdagent_clipboards_init(struct vdagent_x11 *x11,
> > - struct udscs_connection *conn)
> > +static void
> > +vdagent_clipboards_init(VDAgentClipboards *self)
> > {
> > -#ifdef WITH_GTK
> > -guint sel_id;
> > -#endif
> > +}
> > +
> > +VDAgentClipboards *vdagent_clipboards_new(struct vdagent_x11 *x11)
> > +{
> > +VDAgentClipboards *self = g_object_new(VDAGENT_TYPE_CLIPBOARDS, NULL);
> >
> > -VDAgentClipboards *c;
> > -c = g_new0(VDAgentClipboards, 1);
> > #ifndef WITH_GTK
> > -c->x11 = x11;
> > +self->x11 = x11;
> > #else
> > -c->conn = conn;
> > +guint sel_id;
> >
> > for (sel_id = 0; sel_id < SELECTION_COUNT; sel_id++) {
> > GtkClipboard *clipboard = gtk_clipboard_get(sel_atom[sel_id]);
> > -c->selections[sel_id].clipboard = clipboard;
> > +self->selections[sel_id].clipboard = clipboard;
> > g_signal_connect(G_OBJECT(clipboard), "owner-change",
> > - G_CALLBACK(clipboard_owner_change_cb), c);
> > + G_CALLBACK(clipboard_owner_change_cb), self);
> > }
> > #endif
> >
> > -return c;
> > +return self;
> > }
> >
> > -void vdagent_clipboards_finalize(VDAgentClipboards *c, gboolean conn_alive)
> > +void
> > +vdagent_clipboards_set_conn(VDAgentClipboards *self, struct
> > udscs_connection
> > *conn)
> > +{
> > +self->conn = conn;
> > +}
> > +
> > +static void vdagent_clipboards_dispose(GObject *obj)
> > {
> > #ifdef WITH_GTK
> > +VDAgentClipboards *self = VDAGENT_CLIPBOARDS(obj);
> > guint sel_id;
> > +
> > for (sel_id = 0; sel_id < SELECTION_COUNT; sel_id++)
> > -
> > g_signal_handlers_disconnect_by_func(c->selections[sel_id].clipboard,
> > -G_CALLBACK(clipboard_owner_change_cb), c);
> > +
> > g_signal_handlers_disconnect_by_func(self->selections[sel_id].clipboard,
> > +G_CALLBACK(clipboard_owner_change_cb), self);
> >
> > -if (conn_alive == FALSE)
> > -c->conn = NULL;
> > -vdagent_clipboards_release_all(c);
> > +if (self->conn)
> > +vdagent_clipboards_release_all(self);
> > #endif
> > +}
> > +
> > +static void
> > +vdagent_clipboards_class_init(VDAgentClipboardsClass *klass)
> > +{
> > +GObjectClass *oclass = G_OBJECT_CLASS(klass);
> >
> > -g_free(c);
> > +oclass->dispose = vdagent_clipboards_dispose;
> > }
> > diff --git a/src/vdagent/clipboard.h b/src/vdagent/clipboard.h
> > index f819b49..cd8ea
Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL
Hi
On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku wrote:
>
> On Fri, Mar 22, 2019 at 2:57 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > When this capability is negoticated by both the client & the agent,
> > the clipboard grab messages have an associated serial counter.
> >
> > The serial is reset to 0 upon client connection.
> >
> > The counter is increment by 1 on each grab message, by both sides.
> >
> > The sender of the message with the highest serial should be the
> > clipboard grab owner, and the current session serial should be
> > updated.
> >
> > If a lower serial than the current session serial is received, the
> > grab should be discarded.
> >
> > Whenever two grabs share the same serial, the one coming from the
> > client should have a higher priority and the client should gain the
> > clipboard ownership.
>
> Why should the client be the one with a higher priority?
No strong reason, there has to be a winner in this case: both side are
taking the grab simultaneously, we have to decide which one actually
holds it in the end. Other ideas?
>
> If you look at James' case again:
> with you proposed change, the clipboard manager would take over if a
> race occurs, but the clipboard manager usually supports only a limited
> number of targets.
> For example, you copy a graph in Excel, (regrab and race occurs),
> clipboard manager from the client wins and sets the clipboard, now
> you're able to paste the graph only as a non-editable image.
If there is a clipboard manager on the client side, it's the client
desire to do such transformation, isn't it?
>
> So to provide an "uninterrupted" experience, I think that the
> component with keyboard focus should actually get the priority.
You are working against the clipboard manager in this case, which may
do as much harm as good.
> >
> > No special treatement is done for the unlikely case of overflowing the
> > counter. It may temporarily inverse the priority, until both side have
> > overflown and/or synchronized.
> >
> > Note: this mechanism isn't aiming at making "the most recent" (as in
> > time) side gaining the ownership. One side sending subsequent grab
> > messages earlier will likely take the ownership over a side sending a
> > single message simultaneously the other way. It only clears the
> > situation where both side believe that the other is the current
> > clipboard owner, by having a global ordering and priority in case of
> > serial conflict.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > spice/vd_agent.h | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index 862cb5c..9ae3cc7 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab {
> > #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > uint8_t selection;
> > uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > +#endif
> > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > +uint32_t serial;
> > #endif
> > uint32_t types[0];
> > } VDAgentClipboardGrab;
> > @@ -288,6 +291,7 @@ enum {
> > VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
> > VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB,
> > +VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL,
> > VD_AGENT_END_CAP,
> > };
> >
> > --
> > 2.21.0.4.g36eb1cb9cf
> >
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 2/2] clipboard: do not release between remote grabs
Hi
On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku wrote:
>
> Hi,
>
> On Thu, Mar 21, 2019 at 1:21 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > Delay the release events for 0.5 sec. If no further grab comes in,
> > then release the grab. Otherwise, let's skip the release. This avoids
> > some races with clipboard managers.
> >
> > Related to:
> > https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> >
> > Signed-off-by: Marc-André Lureau
>
> In the 0.5 second period, any requests from apps in the client for
> clipboard data should be ignored since the vdagent can't provide the
> data any more, so we shouldn't request it.
> So I would add a condition to clipboard_get():
> if (s->clipboard_release_delay[selection]) {
> SPICE_DEBUG("...");
> return;
> }
yes, thanks
> > ---
> > src/spice-gtk-session.c | 80 -
> > 1 file changed, 72 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 0e748b6..d73a44b 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
> > gbooleanclip_hasdata[CLIPBOARD_LAST];
> > gbooleanclip_grabbed[CLIPBOARD_LAST];
> > gbooleanclipboard_by_guest[CLIPBOARD_LAST];
> > +guint clipboard_release_delay[CLIPBOARD_LAST];
> > /* auto-usbredir related */
> > gbooleanauto_usbredir_enable;
> > int auto_usbredir_reqs;
> > @@ -95,6 +96,7 @@ struct _SpiceGtkSessionPrivate {
> >
> > /* -- */
> > /* Prototypes for private functions */
> > +static void clipboard_release(SpiceGtkSession *self, guint selection);
> > static void clipboard_owner_change(GtkClipboard *clipboard,
> > GdkEventOwnerChange *event,
> > gpointer user_data);
> > @@ -247,6 +249,23 @@ static void spice_gtk_session_dispose(GObject *gobject)
> > G_OBJECT_CLASS(spice_gtk_session_parent_class)->dispose(gobject);
> > }
> >
> > +static void clipboard_release_delay_remove(SpiceGtkSession *self, guint
> > selection,
> > + gboolean release_if_delayed)
> > +{
> > +SpiceGtkSessionPrivate *s = self->priv;
> > +
> > +if (!s->clipboard_release_delay[selection])
> > +return;
> > +
> > +if (release_if_delayed) {
> > +SPICE_DEBUG("delayed clipboard release, sel:%u", selection);
> > +clipboard_release(self, selection);
> > +}
> > +
> > +g_source_remove(s->clipboard_release_delay[selection]);
> > +s->clipboard_release_delay[selection] = 0;
> > +}
> > +
> > static void spice_gtk_session_finalize(GObject *gobject)
> > {
> > SpiceGtkSession *self = SPICE_GTK_SESSION(gobject);
> > @@ -256,6 +275,7 @@ static void spice_gtk_session_finalize(GObject *gobject)
> > /* release stuff */
> > for (i = 0; i < CLIPBOARD_LAST; ++i) {
> > g_clear_pointer(>clip_targets[i], g_free);
> > +clipboard_release_delay_remove(self, i, true);
> > }
> >
> > /* Chain up to the parent class */
> > @@ -815,6 +835,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
> > guint selection,
> > int m, n;
> > int num_targets = 0;
> >
> > +clipboard_release_delay_remove(self, selection, false);
> > +
> > cb = get_clipboard_from_selection(s, selection);
> > g_return_val_if_fail(cb != NULL, FALSE);
> >
> > @@ -1044,17 +1066,12 @@ static gboolean clipboard_request(SpiceMainChannel
> > *main, guint selection,
> > return TRUE;
> > }
> >
> > -static void clipboard_release(SpiceMainChannel *main, guint selection,
> > - gpointer user_data)
> > +static void clipboard_release(SpiceGtkSession *self, guint selection)
> > {
> > -g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
> > -
> > -SpiceGtkSession *self = user_data;
> > SpiceGtkSessionPrivate *s = self->priv;
> > GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
> >
> > -if (!clipboard)
> > -return;
> > +g_return_if_fail(clipboard != NULL);
> >
&g
Re: [Spice-devel] [PATCH spice-gtk 0/2] clipboard: skip release between grabs
Hi On Sun, Mar 24, 2019 at 6:50 PM Jakub Janku wrote: > > Hi, > > On Fri, Mar 22, 2019 at 10:15 AM Victor Toso wrote: > > > > Hi James, > > > > If you have the time, could you give this a try? > > CC-ing James > > > > Cheers, > > > > On Thu, Mar 21, 2019 at 01:21:32PM +0100, marcandre.lur...@redhat.com wrote: > > > From: Marc-André Lureau > > > > > > Hi, > > > > > > There has been several reports of clipboard issues that seem related > > > to clipboard managers interactions. Although it's not crystal clear > > > what are the problems, > > Hmmm. What's left to be cleared? Don't ask me to clarify bugs I don't fully understand and can't reproduce reliably... All I understand is that some clipboard managers react in various ways to release events, and the spice protocol isn't able to cope nicely with grab races, which creates conflicting situations. > > >> we realized the spice protocol does't handle > > > well conflicting grabs coming simultaneously from client and remote. > > > Each other believe the other side is the owner. This situation > > > is rare, but with a clipboard manager that reacts to release events, > > > it may be more easily reproducible. > > > > > > Both agent & client send release between grabs today. This series > > > propose to stop releasing between grabs on client side, and adds a > > > 0.5s delay for the release event coming from the agent side, to filter > > > out release between grabs. > > Delaying the release for 0.5 seconds seems wrong to me. Given it's > just a temporary workaround, I'm fine with it. Keeping in mind that release event is not usually vital for a functioning clipboard sharing. There are not that many release events. We have those created by spice implementations that are easily removable as done in my series. And then those created by the users which are impacted by the usual communication (be it IPC or over the network). Adding 0.5s isn't going to change much the user experience for something that is already margninal to a functioning clipboard. > But it should be tested by someone who's experienced the issues in the > real world to see if the delay is sufficient and doesn't cause any > other problems. > > > > > > Further protocol improvements should remove the need for a delay, and > > > solve the conflicting situation in all cases. This will come in a > > > future series. > > > > > > Marc-André Lureau (2): > > > clipboard: do not release between client grabs > > > clipboard: do not release between remote grabs > > > > > > src/spice-gtk-session.c | 101 +++- > > > 1 file changed, 80 insertions(+), 21 deletions(-) > > > > > > -- > > > 2.21.0.4.g36eb1cb9cf > > > > > > _______ > > > Spice-devel mailing list > > > Spice-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > Cheers, > Jakub > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/2] clipboard: do not release between client grabs
Hi
On Sun, Mar 24, 2019 at 6:50 PM Jakub Janku wrote:
>
> Hi,
>
> On Thu, Mar 21, 2019 at 1:21 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > On the client side, whenever the grab owner changes (and the clipboard
> > was previously grabbed), spice-gtk sends a clipboard release followed
> > immediately by a new grab. But some clipboard managers on the remote
> > side react to clipboard release events by taking a clipboard grab,
> > presumably to avoid empty clipboards.
> >
> > The two grabs, coming from the client and from the remote sides, will
> > race in both directions, which may confuse the client & remote side,
> > as both believe the other side is the current grab owner, and thus
> > further clipboard data requests are likely to fail.
> >
> > Let's avoid sending a release event when re-grabing.
> >
> > The race described above may still happen in other rare circunstances,
> > and will require a protocol change. To avoid the conflict, a discussed
> > solution could use a clipboard serial number.
> >
> > Tested with current linux & windows vdagent. Looking at earlier
> > version of the code, it doesn't seem like subsequent grabs will be
> > treated as an error.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/spice-gtk-session.c | 21 -
> > 1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..0e748b6 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard
> > *clipboard,
> > g_return_if_fail(selection != -1);
> >
> > if (s->clip_grabbed[selection]) {
> > -SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms",
> > n_atoms);
> > -return;
> > +SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms",
> > n_atoms);
> > }
> >
> > /* Set all Atoms that matches our current protocol implementation */
> > @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard
> > *clipboard,
> > return;
> > }
> >
> > -/* In case we sent a grab to the agent, we need to release it now as
> > - * previous clipboard data should not be reachable anymore */
> > -if (s->clip_grabbed[selection]) {
> > -s->clip_grabbed[selection] = FALSE;
> > -if (spice_main_channel_agent_test_capability(s->main,
> > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > -spice_main_channel_clipboard_selection_release(s->main,
> > selection);
> > -}
> > -}
>
> If event->owner is NULL, the clipboard is empty at the moment and this
> function exits without requesting the new targets. So in this case, we
> should still send release to the agent, otherwise the guest might
> think that clipboard data can be provided while the clipboard in the
> client is empty for a long time. Pasting data in the guest would
> result into an invalid request being sent to spice-gtk.
This is going into undocumented territories. But if what you say is
true in commit 9af2c481b74077ab7d6cb9d4bf589f9855a302f5, then yes,
client should probably release the clipboard.
I don't quite understand why gtk+ would allow NEW_OWNER, with owner ==
NULL and empty clipboard. It sounds more like a bug to me.
Would this be enough ?
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 5b2c27c..3dbcae6 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -671,7 +671,11 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
return;
}
-if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
+if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER ||
+#ifdef GDK_WINDOWING_X11
+(!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default()))
+#endif
+) {
if (s->clip_grabbed[selection]) {
/* grab was sent to the agent, so release it */
s->clip_grabbed[selection] = FALSE;
@@ -690,13 +694,6 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
s->clipboard_by_guest[selection] = FALSE;
-#ifdef GDK_WINDOWING_X11
-if (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
-s->clip_hasdata[selection] = FALSE;
-return;
-}
-#endif
-
> > -
> > -/* We are mostly interested when owner has changed in which case
> > - * we would like to let agent know about new clipboard data. */
> >
Re: [Spice-devel] [PATCH linux/vd-agent 02/11] session: free active session
Hi
On Sun, Mar 24, 2019 at 6:50 PM Jakub Janku wrote:
>
> On Fri, Mar 22, 2019 at 4:13 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > According to sd_seat_get_active(2), you must free() the pointer.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/vdagentd/systemd-login.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/vdagentd/systemd-login.c b/src/vdagentd/systemd-login.c
> > index a11b66d..0b2dd0a 100644
> > --- a/src/vdagentd/systemd-login.c
> > +++ b/src/vdagentd/systemd-login.c
> > @@ -266,6 +266,7 @@ const char *session_info_get_active_session(struct
> > session_info *si)
> > int r;
> > char *old_session = si->session;
> >
> > +free(si->session);
>
> The session does get freed just a couple of lines below here.
Right, my bad!
However, it should use free() and not g_free(). I'll update the patch.
thanks
>
> Cheers,
> Jakub
>
> > si->session = NULL;
> > r = sd_seat_get_active("seat0", >session, NULL);
> > /* ENOENT happens when a seat is switching from one session to another
> > */
> > --
> > 2.21.0.4.g36eb1cb9cf
> >
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v3] smartcard: Warn if multiple readers are detected
On Fri, Feb 22, 2019 at 11:40 AM Christophe Fergeau wrote:
>
> spice-server does not deal properly with multiple smartcard readers,
> only the first one will be working. Add a warning when this happens to
> make it easier to diagnose such issues.
>
> Signed-off-by: Christophe Fergeau
ack
> ---
> src/smartcard-manager.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> index ceecfdc7..35bb2757 100644
> --- a/src/smartcard-manager.c
> +++ b/src/smartcard-manager.c
> @@ -389,6 +389,24 @@ typedef struct {
> GError *err;
> } SmartcardManagerInitArgs;
>
> +
> +static void smartcard_reader_free(gpointer data)
> +{
> +g_boxed_free(SPICE_TYPE_SMARTCARD_READER, data);
> +}
> +
> +/* spice-server only supports one smartcard reader being in use */
> +static void smartcard_check_reader_count(void)
> +{
> +GList *readers;
> +
> +readers =
> spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
> +if (g_list_length(readers) > 1) {
> +g_warning("Multiple smartcard readers are plugged in, only the first
> one will be shared with the VM");
> +}
> +g_list_free_full(readers, smartcard_reader_free);
> +}
> +
> static gboolean smartcard_manager_init(SmartcardManagerInitArgs *args)
> {
> gchar *emul_args = NULL;
> @@ -442,6 +460,7 @@ init:
> "Failed to initialize smartcard");
> goto end;
> }
> +smartcard_check_reader_count();
>
> retval = TRUE;
>
> --
> 2.20.1
>
> _______
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi
On Fri, Mar 15, 2019 at 4:43 PM Jakub Janku wrote:
>
> Hi,
>
> On Thu, Mar 14, 2019 at 6:18 PM Marc-André Lureau
> wrote:
> >
> > Hi
> >
> > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků wrote:
> > >
> > > If two grab messages in opposite directions "meet" on their way
> > > to their destinations, we end up in a state when both spice-gtk
> > > and spice-vdagent think that the other component can provide
> > > clipboard data. As a consequence of this, neither of them can.
> > >
> > > This is a bug in the spice-protocol. To mitigate the issue,
> > > accept grab only from the side that currently has keyboard focus,
> > > this means:
> > > (1) spice-gtk has focus ==>
> > > * accept grabs from vdagent,
> > > * ignore grabs from other apps running in the host
> > > (2) spice-gtk doesn't have focus ==>
> > > * accept grabs from other apps running in the host
> > > * ignore grabs from vdagent
> > >
> > > The check in clipboard_owner_change() is X11-specific,
> > > because on Wayland, spice-gtk is first notified about the
> > > owner-change once it gains focus.
> > >
> > > The check in clipboard_grab() can be generic.
> > > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > > while not having focus doesn't have any effect anyway,
> > > only focused clients can set clipboard.
> > >
> > > With this patch, the race can still occur around the time
> > > when focus changes (rare, but possible), on X11 as well as Wayland.
> > >
> > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > >
> > > Signed-off-by: Jakub Janků
> > > ---
> > >
> > > Victor, half of this patch is basically yours,
> > > so feel free to add signed-off or something,
> > > in the case this gets upstream :)
> > >
> > > ---
> > > src/spice-gtk-session.c | 13 +
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index b48f92a..7b7370c 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard
> > > *clipboard,
> > > s->clip_hasdata[selection] = FALSE;
> > > return;
> > > }
> > > +
> > > +if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > +spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > > +"ignoring grab from other app", __FUNCTION__);
> > > +return;
> > > +}
> > > #endif
> >
> > This will break clipboard managers interactions, which may take the
> > clipboard content, save it and/or modify it.
>
> Depends on the implementation of the given clipboard manager, I'd say.
> I tried the "clipboard indicator" you're using. Seems like no problem there :)
without, and with that patch I suppose
> >
> > >
> > > s->clip_hasdata[selection] = TRUE;
> > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel
> > > *main, guint selection,
> > > cb = get_clipboard_from_selection(s, selection);
> > > g_return_val_if_fail(cb != NULL, FALSE);
> > >
> > > +if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > > +"ignoring grab from guest agent", __FUNCTION__);
> > > +return FALSE;
> > > +}
> >
> >
> > Beside automation, the cursor alone may easily create a new clipboard
> > content which won't be available to the client side (the auto-grab may
> > fail to follow cursor etc).
> >
> > It's a bit unclear why it's not X11 specific but for client side
> > change it is, this could deserve a code comment.
>
> Tried to describe that in the commit log. I could add a comment in the
> code as well.
yes, please
> >
> > All in all, this feels weak and breaks some legitimate cases.
> >
> > I am not very strongly against this, as I understand it may help with
> > some races we discussed,
>
> Is this an ack or nack?
If you ask me,
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi
On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků wrote:
>
> If two grab messages in opposite directions "meet" on their way
> to their destinations, we end up in a state when both spice-gtk
> and spice-vdagent think that the other component can provide
> clipboard data. As a consequence of this, neither of them can.
>
> This is a bug in the spice-protocol. To mitigate the issue,
> accept grab only from the side that currently has keyboard focus,
> this means:
> (1) spice-gtk has focus ==>
> * accept grabs from vdagent,
> * ignore grabs from other apps running in the host
> (2) spice-gtk doesn't have focus ==>
> * accept grabs from other apps running in the host
> * ignore grabs from vdagent
>
> The check in clipboard_owner_change() is X11-specific,
> because on Wayland, spice-gtk is first notified about the
> owner-change once it gains focus.
>
> The check in clipboard_grab() can be generic.
> Note that on Wayland, calling gtk_clipboard_set_with_owner()
> while not having focus doesn't have any effect anyway,
> only focused clients can set clipboard.
>
> With this patch, the race can still occur around the time
> when focus changes (rare, but possible), on X11 as well as Wayland.
>
> Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Jakub Janků
> ---
>
> Victor, half of this patch is basically yours,
> so feel free to add signed-off or something,
> in the case this gets upstream :)
>
> ---
> src/spice-gtk-session.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..7b7370c 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> s->clip_hasdata[selection] = FALSE;
> return;
> }
> +
> +if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> +spice_gtk_session_get_keyboard_has_focus(self)) {
> +SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> +"ignoring grab from other app", __FUNCTION__);
> +return;
> +}
> #endif
This will break clipboard managers interactions, which may take the
clipboard content, save it and/or modify it.
>
> s->clip_hasdata[selection] = TRUE;
> @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
> guint selection,
> cb = get_clipboard_from_selection(s, selection);
> g_return_val_if_fail(cb != NULL, FALSE);
>
> +if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> +SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> +"ignoring grab from guest agent", __FUNCTION__);
> +return FALSE;
> +}
Beside automation, the cursor alone may easily create a new clipboard
content which won't be available to the client side (the auto-grab may
fail to follow cursor etc).
It's a bit unclear why it's not X11 specific but for client side
change it is, this could deserve a code comment.
All in all, this feels weak and breaks some legitimate cases.
I am not very strongly against this, as I understand it may help with
some races we discussed, but it feels like the problem is elsewhere
and we need a better solution to prevent the race from happening.
I haven't read the bug reports: this kind of workaround needs a
description of a broken use case (not a theoretical description of a
race that "never" happen in practice).
> +
> for (n = 0; n < ntypes; ++n) {
> found = FALSE;
> for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku wrote: > > Hi, > > On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau > wrote: > > > > Hi > > > > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku wrote: > > > > > > Hi, > > > > > > thanks for having a look! > > > > > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > > > wrote: > > > > > > > > Hi > > > > > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků wrote: > > > > > > > > > > If two grab messages in opposite directions "meet" on their way > > > > > to their destinations, we end up in a state when both spice-gtk > > > > > and spice-vdagent think that the other component can provide > > > > > clipboard data. As a consequence of this, neither of them can. > > > > > > > > > > This is a bug in the spice-protocol. To mitigate the issue, > > > > > > > > With such as statement, you should provide detailed analysis, and > > > > strong arguments for workarounds. > > > > > > Depends on what you consider to be "detailed analysis". Since you > > > correctly restated the problem with your own words, it seems like my > > > rather short description served its purpose :) > > > Also please note that I've tried to provide a more detailed analysis > > > of the issue reported by James Harvey earlier in [0] > > > > > > [0] > > > https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html > > > > > > As for the arguments, I've actually expressed my main argument in the > > > cover letter, have you read it? > > > "Intention of these patches is to make spice-gtk > > > behave reasonably well with older agents." > > > I would like to have this patch reviewed with this in mind. > > > So this patch NOT trying to be a final solution to the problem. > > > > > > If you look at the race condition from the standpoint of spice-gtk, > > > the situation looks the following: > > > 1) spice-gtk sends grab > > > 2) spice-gtk receives grab > > > You can't tell if it's a race or not. The normal behaviour can look the > > > same. > > > > > > Now, if you consider fix in spice-gtk only (to make older vdagents > > > play nicely with new spice-gtk), I see these possible mitigations: > > > a) measure time between 1) and 2) and discard one grab if the elapsed > > > time is too short: > > > this solution is rather empirical and seems unreliable to me > > > b) accept grab only from one side at a moment - that is what this patch > > > does > > > > > > > > > > > I think you are describing this conflicting situation: (all messages > > > > are asynchronous here, A & B are either client or remote end > > > > interchangeably) > > > > > > > > A->B: grab > > > > B->A: grab > > > > A: handle B grab > > > > B: handle A grab > > > > > > > > Both A think the other end has the grab... > > > > > > > > If we would instead explicitly release the grab, none would have it, > > > > which could be considered an improvement: > > > > > > Yes, a slight improvement. However, the original grabs both in client > > > and guest would be lost, which is unwanted. > > > > > > > > A->B: grab > > > > B->A: grab > > > > A: handle B grab > > > > A->B: release > > > > B: handle A grab > > > > B->A: release > > > > B: handle A release > > > > A: handle B release > > > > > > > > Instead, we should probably have a priority for who wins. I suppose it > > > > should be the client. > > > > > > Not necessarily. Consider James' case: if we make the client win, the > > > clipboard manager in the client would take over the clipboard in the > > > guest. The clipboard manager filters out some of the targets and > > > additionally, the "marching ants" in Excel would disappear. > > > > > > >But how to distinguish the conflict case with > > > > the normal case? Perhaps an incremental counter (age/generation, > > > > whatever you call it) would be enough. > > > > > > This would need a protocol change. > > > So given my note in the cover letter, this comme
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku wrote: > > Hi, > > thanks for having a look! > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > wrote: > > > > Hi > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků wrote: > > > > > > If two grab messages in opposite directions "meet" on their way > > > to their destinations, we end up in a state when both spice-gtk > > > and spice-vdagent think that the other component can provide > > > clipboard data. As a consequence of this, neither of them can. > > > > > > This is a bug in the spice-protocol. To mitigate the issue, > > > > With such as statement, you should provide detailed analysis, and > > strong arguments for workarounds. > > Depends on what you consider to be "detailed analysis". Since you > correctly restated the problem with your own words, it seems like my > rather short description served its purpose :) > Also please note that I've tried to provide a more detailed analysis > of the issue reported by James Harvey earlier in [0] > > [0] > https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html > > As for the arguments, I've actually expressed my main argument in the > cover letter, have you read it? > "Intention of these patches is to make spice-gtk > behave reasonably well with older agents." > I would like to have this patch reviewed with this in mind. > So this patch NOT trying to be a final solution to the problem. > > If you look at the race condition from the standpoint of spice-gtk, > the situation looks the following: > 1) spice-gtk sends grab > 2) spice-gtk receives grab > You can't tell if it's a race or not. The normal behaviour can look the same. > > Now, if you consider fix in spice-gtk only (to make older vdagents > play nicely with new spice-gtk), I see these possible mitigations: > a) measure time between 1) and 2) and discard one grab if the elapsed > time is too short: > this solution is rather empirical and seems unreliable to me > b) accept grab only from one side at a moment - that is what this patch does > > > > > I think you are describing this conflicting situation: (all messages > > are asynchronous here, A & B are either client or remote end > > interchangeably) > > > > A->B: grab > > B->A: grab > > A: handle B grab > > B: handle A grab > > > > Both A think the other end has the grab... > > > > If we would instead explicitly release the grab, none would have it, > > which could be considered an improvement: > > Yes, a slight improvement. However, the original grabs both in client > and guest would be lost, which is unwanted. > > > > A->B: grab > > B->A: grab > > A: handle B grab > > A->B: release > > B: handle A grab > > B->A: release > > B: handle A release > > A: handle B release > > > > Instead, we should probably have a priority for who wins. I suppose it > > should be the client. > > Not necessarily. Consider James' case: if we make the client win, the > clipboard manager in the client would take over the clipboard in the > guest. The clipboard manager filters out some of the targets and > additionally, the "marching ants" in Excel would disappear. > > >But how to distinguish the conflict case with > > the normal case? Perhaps an incremental counter (age/generation, > > whatever you call it) would be enough. > > This would need a protocol change. > So given my note in the cover letter, this comment is quite unrelated > to this patch. I would prefer to discuss the protocol change later. > > > > Did you thought about something along those lines? Would you be able > > to come up with protocol changes for that? > > > > At this point, I think we could use some nice sequence diagrams in the spec! > > > > > > > accept grab only from the side that currently has keyboard focus, > > > this means: > > > (1) spice-gtk has focus ==> > > > * accept grabs from vdagent, > > > * ignore grabs from other apps running in the host > > > > I have a hard time understanding why keyboard focus is related to > > clipboard. clipboard can be interacted with mouse only, or > > independently from any user input. Or from multiple inputs. I try to > > fit the keyboard input this into clipboard interaction issues, and I > > don't think that helps much. > > If spice-gtk has keyboard focus, the user interacts with the guest system. > If it doesn't, user interacts with the client system. > &g
Re: [Spice-devel] [PATCH spice-gtk 3/3] clipboard: invalidate targets request when needed
Hi
On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků wrote:
>
> Targets request is no longer relevant when
> clipboard owner changes since the retrieved targets
> will be outdated.
>
> When the request is no longer relevant,
> invalidate it by pointing its weak ref to NULL.
> As a consequence, free_weak_ref() call returns NULL
> and clipboard_get_targets() exits.
Why not simply check if user_data == last_targets_request?
What does nullify_weak_ref() really helps with?
>
> Signed-off-by: Jakub Janků
> ---
>
> This addresses the same issue that
> was present in spice-vdagent and is already fixed.
>
> ---
> src/spice-gtk-session.c | 39 +--
> 1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 018cb4b..037547a 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
> gbooleanclip_hasdata[CLIPBOARD_LAST];
> gbooleanclip_grabbed[CLIPBOARD_LAST];
> gbooleanclipboard_by_guest[CLIPBOARD_LAST];
> +GWeakRef*last_targets_request[CLIPBOARD_LAST];
> /* auto-usbredir related */
> gbooleanauto_usbredir_enable;
> int auto_usbredir_reqs;
> @@ -537,6 +538,16 @@ static gpointer free_weak_ref(gpointer data)
> return object;
> }
>
> +static void nullify_weak_ref(GWeakRef **weak_ref_p)
> +{
> +gpointer null_object = NULL;
> +
> +if (*weak_ref_p) {
> +g_weak_ref_set(*weak_ref_p, null_object);
> +*weak_ref_p = NULL;
> +}
> +}
> +
> static void clipboard_get_targets(GtkClipboard *clipboard,
>GdkAtom *atoms,
>gint n_atoms,
> @@ -551,23 +562,25 @@ static void clipboard_get_targets(GtkClipboard
> *clipboard,
>
> g_return_if_fail(SPICE_IS_GTK_SESSION(self));
>
> -if (atoms == NULL) {
> -SPICE_DEBUG("Retrieving the clipboard data has failed");
> -return;
> -}
> -
> SpiceGtkSessionPrivate *s = self->priv;
> guint32 types[SPICE_N_ELEMENTS(atom2agent)] = { 0 };
> gint num_types;
> int a;
> int selection;
>
> -if (s->main == NULL)
> -return;
> -
> selection = get_selection_from_clipboard(s, clipboard);
> g_return_if_fail(selection != -1);
>
> +s->last_targets_request[selection] = NULL;
> +
> +if (atoms == NULL) {
> +SPICE_DEBUG("Retrieving the clipboard data has failed");
> +return;
> +}
> +
> +if (s->main == NULL)
> +return;
> +
> if (s->clip_grabbed[selection]) {
> SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms",
> n_atoms);
> return;
> @@ -652,6 +665,8 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> return;
> }
>
> +nullify_weak_ref(>last_targets_request[selection]);
> +
> /* In case we sent a grab to the agent, we need to release it now as
> * previous clipboard data should not be reachable anymore */
> if (s->clip_grabbed[selection]) {
> @@ -690,9 +705,11 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> #endif
>
> s->clip_hasdata[selection] = TRUE;
> -if (s->auto_clipboard_enable && !read_only(self))
> +if (s->auto_clipboard_enable && !read_only(self)) {
> +s->last_targets_request[selection] = get_weak_ref(self);
> gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
> - get_weak_ref(self));
> + s->last_targets_request[selection]);
> +}
> }
>
> typedef struct
> @@ -866,6 +883,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
> guint selection,
> return TRUE;
> }
>
> +nullify_weak_ref(>last_targets_request[selection]);
> +
> if (!gtk_clipboard_set_with_owner(cb,
>targets,
>num_targets,
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi
On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků wrote:
>
> If two grab messages in opposite directions "meet" on their way
> to their destinations, we end up in a state when both spice-gtk
> and spice-vdagent think that the other component can provide
> clipboard data. As a consequence of this, neither of them can.
>
> This is a bug in the spice-protocol. To mitigate the issue,
With such as statement, you should provide detailed analysis, and
strong arguments for workarounds.
I think you are describing this conflicting situation: (all messages
are asynchronous here, A & B are either client or remote end
interchangeably)
A->B: grab
B->A: grab
A: handle B grab
B: handle A grab
Both A think the other end has the grab...
If we would instead explicitly release the grab, none would have it,
which could be considered an improvement:
A->B: grab
B->A: grab
A: handle B grab
A->B: release
B: handle A grab
B->A: release
B: handle A release
A: handle B release
Instead, we should probably have a priority for who wins. I suppose it
should be the client. But how to distinguish the conflict case with
the normal case? Perhaps an incremental counter (age/generation,
whatever you call it) would be enough.
Did you thought about something along those lines? Would you be able
to come up with protocol changes for that?
At this point, I think we could use some nice sequence diagrams in the spec!
> accept grab only from the side that currently has keyboard focus,
> this means:
> (1) spice-gtk has focus ==>
> * accept grabs from vdagent,
> * ignore grabs from other apps running in the host
I have a hard time understanding why keyboard focus is related to
clipboard. clipboard can be interacted with mouse only, or
independently from any user input. Or from multiple inputs. I try to
fit the keyboard input this into clipboard interaction issues, and I
don't think that helps much.
> (2) spice-gtk doesn't have focus ==>
> * accept grabs from other apps running in the host
> * ignore grabs from vdagent
>
> The check in clipboard_owner_change() is X11-specific,
> because on Wayland, spice-gtk is first notified about the
> owner-change once it gains focus.
>
> The check in clipboard_grab() can be generic.
> Note that on Wayland, calling gtk_clipboard_set_with_owner()
> while not having focus doesn't have any effect anyway,
> only focused clients can set clipboard.
>
> With this patch, the race can still occur around the time
> when focus changes (rare, but possible), on X11 as well as Wayland.
>
> Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Jakub Janků
> ---
>
> Victor, half of this patch is basically yours,
> so feel free to add signed-off or something,
> in the case this gets upstream :)
>
> ---
> src/spice-gtk-session.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..7b7370c 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> s->clip_hasdata[selection] = FALSE;
> return;
> }
> +
> +if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
Note, gdk_display_get_default() is probably wrong.
gtk_widget_get_display () is probably better. Not a big deal, needs to
be changed over the whole tree I guess.
> +spice_gtk_session_get_keyboard_has_focus(self)) {
> +SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> +"ignoring grab from other app", __FUNCTION__);
> +return;
> +}
> #endif
>
> s->clip_hasdata[selection] = TRUE;
> @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main,
> guint selection,
> cb = get_clipboard_from_selection(s, selection);
> g_return_val_if_fail(cb != NULL, FALSE);
>
> +if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> +SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> +"ignoring grab from guest agent", __FUNCTION__);
> +return FALSE;
> +}
> +
> for (n = 0; n < ntypes; ++n) {
> found = FALSE;
> for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 06/20] test-stat: Adjust delay checks
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote: > > usleep under Windows does not seem to have the required precision. > Use milliseconds and adjust check times according. > > Signed-off-by: Frediano Ziglio Reviewed-by: Marc-André Lureau > --- > server/tests/stat-test.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/server/tests/stat-test.c b/server/tests/stat-test.c > index e4a83f4f..444ff7e3 100644 > --- a/server/tests/stat-test.c > +++ b/server/tests/stat-test.c > @@ -57,13 +57,13 @@ void TEST_NAME(void) > > stat_init(, "test", CLOCK_MONOTONIC); > stat_start_time_init(_time, ); > -usleep(2); > +usleep(2000); > stat_add(, start_time); > > #ifdef RED_WORKER_STAT > g_assert_cmpuint(info.count, ==, 1); > g_assert_cmpuint(info.min, ==, info.max); > -g_assert_cmpuint(info.min, >=, 2000); > +g_assert_cmpuint(info.min, >=, 200); > g_assert_cmpuint(info.min, <, 1); > #endif > > @@ -71,17 +71,17 @@ void TEST_NAME(void) > > stat_compress_init(, "test", CLOCK_MONOTONIC); > stat_start_time_init(_time, ); > -usleep(2); > +usleep(2000); > stat_compress_add(, start_time, 100, 50); > -usleep(1); > +usleep(1000); > stat_compress_add(, start_time, 1000, 500); > > #ifdef COMPRESS_STAT > g_assert_cmpuint(info.count, ==, 2); > g_assert_cmpuint(info.min, !=, info.max); > -g_assert_cmpuint(info.min, >=, 2000); > +g_assert_cmpuint(info.min, >=, 200); > g_assert_cmpuint(info.min, <, 1); > -g_assert_cmpuint(info.total, >=, 5000); > +g_assert_cmpuint(info.total, >=, 500); > g_assert_cmpuint(info.orig_size, ==, 1100); > g_assert_cmpuint(info.comp_size, ==, 550); > #endif > -- > 2.20.1 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 08/20] sys-socket: Add socket_newpair utility
Hi
On Tue, Mar 5, 2019 at 3:07 PM Frediano Ziglio wrote:
>
> > Hi
> >
> > On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
> > >
> > > Allows to easier port socketpair.
> > > Windows does not have this function, we need to create a pair
> > > using 2 internet sockets and connecting one to the other.
> > >
> > > Signed-off-by: Frediano Ziglio
> > > ---
> > > server/sys-socket.c | 75 +
> > > server/sys-socket.h | 3 ++
> > > 2 files changed, 78 insertions(+)
> > >
> > > diff --git a/server/sys-socket.c b/server/sys-socket.c
> > > index 7ce5dab1..837da18e 100644
> > > --- a/server/sys-socket.c
> > > +++ b/server/sys-socket.c
> > > @@ -209,4 +209,79 @@ SPICE_CONSTRUCTOR_FUNC(socket_win32_init)
> > > WSADATA wsaData;
> > > WSAStartup(MAKEWORD(2, 2), );
> > > }
> > > +
> > > +int socket_newpair(int type, int protocol, int sv[2])
> >
> > overall, looks good.
> >
> > Since it's limited to inet protocols, why not remove "protocol"
> > argument, and call it socket_new_inet_pair() ?
> >
>
> it would make sense too, yes, I'll do.
>
> > If you are using this for worker thread communication, a windows pipe
> > would be a better fit. (and GIO provides stream abstraction, wing has
> > a namedpipe implementation - based on the one we used to have in
> > spice-gtk for controller communication)
> >
>
> We call some Qemu functions which needs sockets, so GIO and windows pipe
> won't work.
>
Can you give some context on expected the usage of this function then?
thanks
> > > +{
> > > +struct sockaddr_in sa, sa2;
> > > +socklen_t addrlen;
> > > +SOCKET s, pairs[2];
> > > +
> > > +if (!sv) {
> > > +return -1;
> > > +}
> > > +
> > > +/* create a listener */
> > > +s = socket(AF_INET, type, 0);
> > > +if (s == INVALID_SOCKET) {
> > > +return -1;
> > > +}
> > > +
> > > +pairs[1] = INVALID_SOCKET;
> > > +
> > > +pairs[0] = socket(AF_INET, type, 0);
> > > +if (pairs[0] == INVALID_SOCKET) {
> > > +goto cleanup;
> > > +}
> > > +
> > > +/* bind to a random port */
> > > +sa.sin_family = AF_INET;
> > > +sa.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> > > +sa.sin_port = 0;
> > > +if (bind(s, (struct sockaddr*) , sizeof(sa)) < 0) {
> > > +goto cleanup;
> > > +}
> > > +if (listen(s, 1) < 0) {
> > > +goto cleanup;
> > > +}
> > > +
> > > +/* connect to kernel choosen port */
> > > +addrlen = sizeof(sa);
> > > +if (getsockname(s, (struct sockaddr*) , ) < 0) {
> > > +goto cleanup;
> > > +}
> > > +if (connect(pairs[0], (struct sockaddr*) , sizeof(sa)) < 0) {
> > > +goto cleanup;
> > > +}
> > > +addrlen = sizeof(sa2);
> > > +pairs[1] = accept(s, (struct sockaddr*) , );
> > > +if (pairs[1] == INVALID_SOCKET) {
> > > +goto cleanup;
> > > +}
> > > +
> > > +/* check proper connection */
> > > +addrlen = sizeof(sa);
> > > +if (getsockname(pairs[0], (struct sockaddr*) , ) < 0) {
> > > +goto cleanup;
> > > +}
> > > +addrlen = sizeof(sa2);
> > > +if (getpeername(pairs[1], (struct sockaddr*) , ) < 0) {
> > > +goto cleanup;
> > > +}
> > > +if (sa.sin_family != sa2.sin_family || sa.sin_port != sa2.sin_port
> > > +|| sa.sin_addr.s_addr != sa2.sin_addr.s_addr) {
> > > +goto cleanup;
> > > +}
> > > +
> > > +closesocket(s);
> > > +sv[0] = pairs[0];
> > > +sv[1] = pairs[1];
> > > +return 0;
> > > +
> > > +cleanup:
> > > +socket_win32_set_errno();
> > > +closesocket(s);
> > > +closesocket(pairs[0]);
> > > +closesocket(pairs[1]);
> > > +return -1;
> > > +}
> > > #endif
> > > diff --git a/server/sys-socket.h b/server/sys-socket.h
> > > index 65062571..3a3b7878 100644
> > > --- a/server/sys-socket.h
> > > +++ b/server/sys-socket.h
> > > @@ -134,6 +134,9 @@ socket_accept(int sock, struct sockaddr *addr, int
> > > *addrlen)
> > > }
> > > #undef accept
> > > #define accept socket_accept
> > > +
> > > +int socket_newpair(int type, int protocol, int sv[2]);
> > > +#define socketpair(family, type, protocol, sv) socket_newpair(type,
> > > protocol, sv)
> > > #endif
> > >
> > > #endif // RED_SYS_SOCKET_H_
>
> Frediano
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 03/20] Avoids %m in formatting for Windows
Hi
On Tue, Mar 5, 2019 at 3:02 PM Frediano Ziglio wrote:
>
> > Hi
> >
> > On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
> > >
> > > Not supported, %m is a GNU extension of sscanf.
> > >
> > > Signed-off-by: Frediano Ziglio
> > > ---
> > > server/reds.c | 25 -
> > > 1 file changed, 12 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/server/reds.c b/server/reds.c
> > > index d3f73d8e..8c1c10dc 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -3698,8 +3698,7 @@ static const int video_codec_caps[] = {
> > > * @codec: a location to return the parsed codec
> > > * @return the position of the next codec in the string
> > > */
> > > -static const char* parse_next_video_codec(const char *codecs, char
> > > **encoder,
> > > - char **codec)
> > > +static char* parse_next_video_codec(char *codecs, char **encoder, char
> > > **codec)
> > > {
> > > if (!codecs) {
> > > return NULL;
> > > @@ -3708,14 +3707,15 @@ static const char* parse_next_video_codec(const
> > > char *codecs, char **encoder,
> > > if (!*codecs) {
> > > return NULL;
> > > }
> > > -int n;
> > > +int end_encoder, end_codec = -1;
> > > *encoder = *codec = NULL;
> > > -if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec,
> > > ) == 2) {
> > > -// this avoids accepting "encoder:codec" followed by garbage like
> > > "$%*"
> > > -if (codecs[n] != ';' && codecs[n] != '\0') {
> > > -free(*codec);
> > > -*codec = NULL;
> > > -}
> > > +if (sscanf(codecs, "%*[0-9a-zA-Z_]:%n%*[0-9a-zA-Z_];%n",
> > > _encoder,
> > > _codec) == 0
> > > +&& end_codec > 0) {
> > > +codecs[end_encoder - 1] = '\0';
> > > +codecs[end_codec - 1] = '\0';
> > > +*encoder = codecs;
> > > +*codec = codecs + end_encoder;
> > > +return codecs + end_codec;
> > > }
> > > return codecs + strcspn(codecs, ";");
> > > }
> > > @@ -3732,7 +3732,8 @@ static void
> > > reds_set_video_codecs_from_string(RedsState *reds, const char *codec
> > > }
> > >
> > > video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
> > > -const char *c = codecs;
> > > +char *codecs_copy = g_strdup_printf("%s;", codecs);
> > > +char *c = codecs_copy;
> > > while ( (c = parse_next_video_codec(c, _name, _name)) )
> > > {
> > > uint32_t encoder_index, codec_index;
> > > if (!encoder_name || !codec_name) {
> > > @@ -3755,11 +3756,9 @@ static void
> > > reds_set_video_codecs_from_string(RedsState *reds, const char *codec
> > > g_array_append_val(video_codecs, new_codec);
> > > }
> > >
> > > -/* these are allocated by sscanf, do not use g_free */
> > > -free(encoder_name);
> > > -free(codec_name);
> > > codecs = c;
> > > }
> > > +g_free(codecs_copy);
> > >
> >
> > codecs may refer to free memory after this, I believe.
> >
>
> No, once converted to indexes only indexes are used.
in the warning below
>
> > Would be nice to factor this parsing code out of RedsState and make
> > some unit tests.
> >
>
> Do you mean test-codecs-parsing ?
Ok I missed that one (I expected something that doesn't involve
SpiceServer, but that's fine too)
thanks
>
> >
> > > if (video_codecs->len == 0) {
> > > spice_warning("Failed to set video codecs, input string: '%s'",
> > > codecs);
>
> Frediano
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 01/20] Use proper format strings for spice_log
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
>
> Formatting string should be compatible with GLib.
> GLib uses formatting types compatible with GNU.
> For Linux this is not an issue as both systems (like a printf) and
> GLib one uses the same formatting type. However on Windows they
> differs potentially causing issues.
> This is also make worse as GLib 2.58 changed format attribute from
> __printf__ to gnu_printf (Microsoft compatibility formats like %I64d
> are still supported but you'll get warnings using GCC/Clang
> compilers).
>
> Signed-off-by: Frediano Ziglio
Reviewed-by: Marc-André Lureau
> ---
> server/char-device.c | 3 ++-
> server/gstreamer-encoder.c | 4 ++--
> server/main-channel-client.c | 6 +++---
> server/memslot.c | 2 +-
> server/mjpeg-encoder.c | 6 --
> server/red-channel.c | 6 --
> server/red-client.c | 6 --
> server/red-replay-qxl.c | 9 +
> server/reds.c| 4 ++--
> 9 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 64b41a94..c00e96ef 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -786,7 +786,8 @@ void red_char_device_client_remove(RedCharDevice *dev,
> }
>
> if (dev->priv->clients == NULL) {
> -spice_debug("client removed, memory pool will be freed (%"PRIu64"
> bytes)", dev->priv->cur_pool_size);
> +spice_debug("client removed, memory pool will be freed
> (%"G_GUINT64_FORMAT" bytes)",
> +dev->priv->cur_pool_size);
> write_buffers_queue_free(>priv->write_bufs_pool);
> dev->priv->cur_pool_size = 0;
> }
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 04f0c02f..972c86e6 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1080,10 +1080,10 @@ static void set_gstenc_bitrate(SpiceGstEncoder
> *encoder)
> break;
> }
> default:
> -spice_warning("the %s property has an unsupported type %zu",
> +spice_warning("the %s property has an unsupported type %"
> G_GSIZE_FORMAT,
>prop, param->value_type);
> }
> -spice_debug("setting the GStreamer %s to %"PRIu64, prop, gst_bit_rate);
> +spice_debug("setting the GStreamer %s to %"G_GUINT64_FORMAT, prop,
> gst_bit_rate);
> }
>
> /* A helper for spice_gst_encoder_encode_frame() */
> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index 54be9934..3b6ae269 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -529,8 +529,8 @@ void main_channel_client_handle_pong(MainChannelClient
> *mcc, SpiceMsgPing *ping,
> if (roundtrip <= mcc->priv->latency) {
> // probably high load on client or server result with incorrect
> values
> red_channel_debug(red_channel_client_get_channel(rcc),
> - "net test: invalid values, latency %" PRIu64
> - " roundtrip %" PRIu64 ". assuming high"
> + "net test: invalid values, latency %"
> G_GUINT64_FORMAT
> + " roundtrip %"G_GUINT64_FORMAT". assuming high"
>"bandwidth", mcc->priv->latency, roundtrip);
> mcc->priv->latency = 0;
> mcc->priv->net_test_stage = NET_TEST_STAGE_INVALID;
> @@ -542,7 +542,7 @@ void main_channel_client_handle_pong(MainChannelClient
> *mcc, SpiceMsgPing *ping,
> / (roundtrip - mcc->priv->latency);
> mcc->priv->net_test_stage = NET_TEST_STAGE_COMPLETE;
> red_channel_debug(red_channel_client_get_channel(rcc),
> - "net test: latency %f ms, bitrate %"PRIu64" bps
> (%f Mbps)%s",
> + "net test: latency %f ms, bitrate
> %"G_GUINT64_FORMAT" bps (%f Mbps)%s",
>(double)mcc->priv->latency / 1000,
>mcc->priv->bitrate_per_sec,
>(double)mcc->priv->bitrate_per_sec / 1024 / 1024,
> diff --git a/server/memslot.c b/server/memslot.c
> index 97311b2e..91ea53bd 100644
> --- a/server/memslot.c
> +++ b/server/memslot.c
> @@ -105,7 +105,7 @@ void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL
> addr, uint32_t add_size
> slo
Re: [Spice-devel] [PATCH spice-server v4 02/20] build: Detect Windows build and change some definitions
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote: > > Windows needs some specific setting to use network. > > Signed-off-by: Frediano Ziglio autotools only? Reviewed-by: Marc-André Lureau > --- > configure.ac | 20 +++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index 604a41b2..f8b41f37 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -68,6 +68,20 @@ case $host_cpu in > SPICE_WARNING([spice-server on non-x86_64 architectures has not been > extensively tested]) > esac > > +AC_MSG_CHECKING([for native Win32]) > +case "$host_os" in > + *mingw*|*cygwin*) > +os_win32=yes > +dnl AI_ADDRCONFIG and possibly some other code require at least Vista > +AC_DEFINE([_WIN32_WINNT], [0x600], [Minimal Win32 version])] > +;; > + *) > +os_win32=no > +;; > +esac > +AC_MSG_RESULT([$os_win32]) > +AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) > + > dnl = > dnl Check optional features > SPICE_CHECK_SMARTCARD > @@ -154,6 +168,9 @@ AC_CHECK_LIB(rt, clock_gettime, LIBRT="-lrt") > AC_SUBST(LIBRT) > > AS_VAR_APPEND([SPICE_NONPKGCONFIG_LIBS], [" -pthread $LIBM $LIBRT"]) > +AS_IF([test "x$os_win32" = "xyes"], [ > +AS_VAR_APPEND([SPICE_NONPKGCONFIG_LIBS], [" -lws2_32"]) > +]) > > SPICE_REQUIRES="" > > @@ -176,7 +193,8 @@ PKG_CHECK_MODULES([GOBJECT2], [gobject-2.0 >= > $GLIB2_REQUIRED]) > AS_VAR_APPEND([SPICE_REQUIRES], [" gobject-2.0 >= $GLIB2_REQUIRED"]) > > #used only by tests > -PKG_CHECK_MODULES([GIO_UNIX], [gio-unix-2.0 >= $GLIB2_REQUIRED]) > +AS_IF([test "x$os_win32" != "xyes"], > + PKG_CHECK_MODULES([GIO_UNIX], [gio-unix-2.0 >= $GLIB2_REQUIRED])) > > PIXMAN_REQUIRED=0.17.7 > PKG_CHECK_MODULES(PIXMAN, pixman-1 >= $PIXMAN_REQUIRED) > -- > 2.20.1 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 09/20] net-utils: Use socket compatibility layer
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
>
> Signed-off-by: Frediano Ziglio
You could easily improve the commit message, as it doesn't seem to do
what it says.
> ---
> server/net-utils.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/server/net-utils.c b/server/net-utils.c
> index 802509a4..ad66a328 100644
> --- a/server/net-utils.c
> +++ b/server/net-utils.c
> @@ -35,6 +35,7 @@
> #include
>
> #include "net-utils.h"
> +#include "sys-socket.h"
>
> /**
> * red_socket_set_keepalive:
> @@ -101,6 +102,7 @@ bool red_socket_set_no_delay(int fd, bool no_delay)
> */
> bool red_socket_set_non_blocking(int fd, bool non_blocking)
> {
> +#ifndef _WIN32
> int flags;
>
> if ((flags = fcntl(fd, F_GETFL)) == -1) {
> @@ -120,6 +122,15 @@ bool red_socket_set_non_blocking(int fd, bool
> non_blocking)
> }
>
> return true;
> +#else
Please invert the condition blocks. The code added is specific to WIN32.
> +u_long ioctl_nonblocking = 1;
> +
> +if (ioctlsocket(fd, FIONBIO, _nonblocking) != 0) {
> +spice_warning("ioctlsocket(FIONBIO) failed, %d", WSAGetLastError());
> +return false;
> +}
> +return true;
> +#endif
> }
>
> /**
> --
> 2.20.1
>
> _______
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 08/20] sys-socket: Add socket_newpair utility
Hi
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
>
> Allows to easier port socketpair.
> Windows does not have this function, we need to create a pair
> using 2 internet sockets and connecting one to the other.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/sys-socket.c | 75 +
> server/sys-socket.h | 3 ++
> 2 files changed, 78 insertions(+)
>
> diff --git a/server/sys-socket.c b/server/sys-socket.c
> index 7ce5dab1..837da18e 100644
> --- a/server/sys-socket.c
> +++ b/server/sys-socket.c
> @@ -209,4 +209,79 @@ SPICE_CONSTRUCTOR_FUNC(socket_win32_init)
> WSADATA wsaData;
> WSAStartup(MAKEWORD(2, 2), );
> }
> +
> +int socket_newpair(int type, int protocol, int sv[2])
overall, looks good.
Since it's limited to inet protocols, why not remove "protocol"
argument, and call it socket_new_inet_pair() ?
If you are using this for worker thread communication, a windows pipe
would be a better fit. (and GIO provides stream abstraction, wing has
a namedpipe implementation - based on the one we used to have in
spice-gtk for controller communication)
> +{
> +struct sockaddr_in sa, sa2;
> +socklen_t addrlen;
> +SOCKET s, pairs[2];
> +
> +if (!sv) {
> +return -1;
> +}
> +
> +/* create a listener */
> +s = socket(AF_INET, type, 0);
> +if (s == INVALID_SOCKET) {
> +return -1;
> +}
> +
> +pairs[1] = INVALID_SOCKET;
> +
> +pairs[0] = socket(AF_INET, type, 0);
> +if (pairs[0] == INVALID_SOCKET) {
> +goto cleanup;
> +}
> +
> +/* bind to a random port */
> +sa.sin_family = AF_INET;
> +sa.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +sa.sin_port = 0;
> +if (bind(s, (struct sockaddr*) , sizeof(sa)) < 0) {
> +goto cleanup;
> +}
> +if (listen(s, 1) < 0) {
> +goto cleanup;
> +}
> +
> +/* connect to kernel choosen port */
> +addrlen = sizeof(sa);
> +if (getsockname(s, (struct sockaddr*) , ) < 0) {
> +goto cleanup;
> +}
> +if (connect(pairs[0], (struct sockaddr*) , sizeof(sa)) < 0) {
> +goto cleanup;
> +}
> +addrlen = sizeof(sa2);
> +pairs[1] = accept(s, (struct sockaddr*) , );
> +if (pairs[1] == INVALID_SOCKET) {
> +goto cleanup;
> +}
> +
> +/* check proper connection */
> +addrlen = sizeof(sa);
> +if (getsockname(pairs[0], (struct sockaddr*) , ) < 0) {
> +goto cleanup;
> +}
> +addrlen = sizeof(sa2);
> +if (getpeername(pairs[1], (struct sockaddr*) , ) < 0) {
> +goto cleanup;
> +}
> +if (sa.sin_family != sa2.sin_family || sa.sin_port != sa2.sin_port
> +|| sa.sin_addr.s_addr != sa2.sin_addr.s_addr) {
> +goto cleanup;
> +}
> +
> +closesocket(s);
> +sv[0] = pairs[0];
> +sv[1] = pairs[1];
> +return 0;
> +
> +cleanup:
> +socket_win32_set_errno();
> +closesocket(s);
> +closesocket(pairs[0]);
> +closesocket(pairs[1]);
> +return -1;
> +}
> #endif
> diff --git a/server/sys-socket.h b/server/sys-socket.h
> index 65062571..3a3b7878 100644
> --- a/server/sys-socket.h
> +++ b/server/sys-socket.h
> @@ -134,6 +134,9 @@ socket_accept(int sock, struct sockaddr *addr, int
> *addrlen)
> }
> #undef accept
> #define accept socket_accept
> +
> +int socket_newpair(int type, int protocol, int sv[2]);
> +#define socketpair(family, type, protocol, sv) socket_newpair(type,
> protocol, sv)
> #endif
>
> #endif // RED_SYS_SOCKET_H_
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 07/20] sys-socket: Introduce some utility to make sockets more portable
Hi
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
>
> Between Unix and Windows socket are quite different:
> - on Windows sockets have a different namespace from C file
> descriptors so you can't use read/write/close or similar functions;
> - errors are not stored in errno but you must be read/write the
> errors with specific function;
> - sometimes sockets are put in non-blocking mode automatically
> calling some functions;
> - SOCKET type is 64 bit on Windows 64 which does not fit technically
> in an int. Is however safe to assume them to fit in an int.
>
> So encapsulate the socket APIs in some definition to make easier
> and more safe to deal with them.
> Where the portability to Windows would make to code more offuscated a Unix
> style was preferred. For instance if errors are detected errno is set from
> Windows socket error instead of changing all code handling.
> Fortunately on Windows Qemu core interface accepts socket (but not
> other types like C file descriptors!).
>
> Signed-off-by: Frediano Ziglio
Reviewed-by: Marc-André Lureau
> ---
> server/Makefile.am | 2 +
> server/sys-socket.c | 212
> server/sys-socket.h | 139 +
> 3 files changed, 353 insertions(+)
> create mode 100644 server/sys-socket.c
> create mode 100644 server/sys-socket.h
>
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 34ec22ad..7f260612 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -166,6 +166,8 @@ libserver_la_SOURCES = \
> stat.h \
> stream-channel.c\
> stream-channel.h\
> + sys-socket.h\
> + sys-socket.c\
> red-stream-device.c \
> red-stream-device.h \
> sw-canvas.c \
> diff --git a/server/sys-socket.c b/server/sys-socket.c
> new file mode 100644
> index ..7ce5dab1
> --- /dev/null
> +++ b/server/sys-socket.c
> @@ -0,0 +1,212 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> + Copyright (C) 2018 Red Hat, Inc.
> +
> + This library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + This library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include
> +#endif
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#ifndef _WIN32
> +#include
> +#include
> +#include
> +#include
> +#include
> +#endif
> +
> +#include
> +
> +#include "sys-socket.h"
> +
> +#ifdef _WIN32
> +// Map Windows socket errors to standard C ones
> +// See
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx
> +void socket_win32_set_errno(void)
> +{
> +int err = EPIPE; // default
> +switch (WSAGetLastError()) {
> +case WSAEWOULDBLOCK:
> +case WSAEINPROGRESS:
> +err = EAGAIN;
> +break;
> +case WSAEINTR:
> +err = EINTR;
> +break;
> +case WSAEBADF:
> +err = EBADF;
> +break;
> +case WSA_INVALID_HANDLE:
> +case WSA_INVALID_PARAMETER:
> +case WSAEINVAL:
> +err = EINVAL;
> +break;
> +case WSAENOTSOCK:
> +err = ENOTSOCK;
> +break;
> +case WSA_NOT_ENOUGH_MEMORY:
> +err = ENOMEM;
> +break;
> +case WSAEPROTONOSUPPORT:
> +case WSAESOCKTNOSUPPORT:
> +case WSAEOPNOTSUPP:
> +case WSAEPFNOSUPPORT:
> +case WSAEAFNOSUPPORT:
> +case WSAVERNOTSUPPORTED:
> +err = ENOTSUP;
> +break;
> +case WSAEFAULT:
> +err = EFAULT;
> +break;
> +case WSAEACCES:
> +err = EACCES;
> +break;
> +case WSAEMFILE:
> +err = EMFILE;
> +break;
> +case WSAENAMETOOLONG:
> +err = ENAMETOOLONG;
> +break;
> +case WSAE
Re: [Spice-devel] [PATCH spice-server v4 06/20] test-stat: Adjust delay checks
Hi On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote: > > usleep under Windows does not seem to have the required precision. > Use milliseconds and adjust check times according. > > Signed-off-by: Frediano Ziglio As discussed previously, g_usleep() would also be a good fit. I would rather use it over the whole code base. > --- > server/tests/stat-test.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/server/tests/stat-test.c b/server/tests/stat-test.c > index e4a83f4f..444ff7e3 100644 > --- a/server/tests/stat-test.c > +++ b/server/tests/stat-test.c > @@ -57,13 +57,13 @@ void TEST_NAME(void) > > stat_init(, "test", CLOCK_MONOTONIC); > stat_start_time_init(_time, ); > -usleep(2); > +usleep(2000); > stat_add(, start_time); > > #ifdef RED_WORKER_STAT > g_assert_cmpuint(info.count, ==, 1); > g_assert_cmpuint(info.min, ==, info.max); > -g_assert_cmpuint(info.min, >=, 2000); > +g_assert_cmpuint(info.min, >=, 200); > g_assert_cmpuint(info.min, <, 1); > #endif > > @@ -71,17 +71,17 @@ void TEST_NAME(void) > > stat_compress_init(, "test", CLOCK_MONOTONIC); > stat_start_time_init(_time, ); > -usleep(2); > +usleep(2000); > stat_compress_add(, start_time, 100, 50); > -usleep(1); > +usleep(1000); > stat_compress_add(, start_time, 1000, 500); > > #ifdef COMPRESS_STAT > g_assert_cmpuint(info.count, ==, 2); > g_assert_cmpuint(info.min, !=, info.max); > -g_assert_cmpuint(info.min, >=, 2000); > +g_assert_cmpuint(info.min, >=, 200); > g_assert_cmpuint(info.min, <, 1); > -g_assert_cmpuint(info.total, >=, 5000); > +g_assert_cmpuint(info.total, >=, 500); > g_assert_cmpuint(info.orig_size, ==, 1100); > g_assert_cmpuint(info.comp_size, ==, 550); > #endif > -- > 2.20.1 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 04/20] windows: Undefine some conflicting preprocessor macros
Hi On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote: > > "interface" and "MAX_MONITORS" are defined in some Windows system > headers causing garbage code to be fed to the compiler. > > Signed-off-by: Frediano Ziglio To avoid the conflict, I suggest renaming those instead. > --- > server/red-qxl.c | 4 > server/reds.c| 6 ++ > 2 files changed, 10 insertions(+) > > diff --git a/server/red-qxl.c b/server/red-qxl.c > index 0dd26b22..9b9b8c17 100644 > --- a/server/red-qxl.c > +++ b/server/red-qxl.c > @@ -40,6 +40,10 @@ > > #include "red-qxl.h" > > +#ifdef _WIN32 > +// undefine conflicting preprocessor macros > +#undef interface > +#endif > > #define MAX_MONITORS_COUNT 16 > > diff --git a/server/reds.c b/server/reds.c > index 8c1c10dc..97023b38 100644 > --- a/server/reds.c > +++ b/server/reds.c > @@ -79,6 +79,12 @@ > #include "net-utils.h" > #include "red-stream-device.h" > > +#ifdef _WIN32 > +// undefine conflicting preprocessor macros > +#undef MAX_MONITORS > +#undef interface > +#endif > + > #define REDS_MAX_STAT_NODES 100 > > static void reds_client_monitors_config(RedsState *reds, > VDAgentMonitorsConfig *monitors_config); > -- > 2.20.1 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4 03/20] Avoids %m in formatting for Windows
Hi
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio wrote:
>
> Not supported, %m is a GNU extension of sscanf.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/reds.c | 25 -
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index d3f73d8e..8c1c10dc 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3698,8 +3698,7 @@ static const int video_codec_caps[] = {
> * @codec: a location to return the parsed codec
> * @return the position of the next codec in the string
> */
> -static const char* parse_next_video_codec(const char *codecs, char **encoder,
> - char **codec)
> +static char* parse_next_video_codec(char *codecs, char **encoder, char
> **codec)
> {
> if (!codecs) {
> return NULL;
> @@ -3708,14 +3707,15 @@ static const char* parse_next_video_codec(const char
> *codecs, char **encoder,
> if (!*codecs) {
> return NULL;
> }
> -int n;
> +int end_encoder, end_codec = -1;
> *encoder = *codec = NULL;
> -if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec,
> ) == 2) {
> -// this avoids accepting "encoder:codec" followed by garbage like
> "$%*"
> -if (codecs[n] != ';' && codecs[n] != '\0') {
> -free(*codec);
> -*codec = NULL;
> -}
> +if (sscanf(codecs, "%*[0-9a-zA-Z_]:%n%*[0-9a-zA-Z_];%n", _encoder,
> _codec) == 0
> +&& end_codec > 0) {
> +codecs[end_encoder - 1] = '\0';
> +codecs[end_codec - 1] = '\0';
> +*encoder = codecs;
> +*codec = codecs + end_encoder;
> +return codecs + end_codec;
> }
> return codecs + strcspn(codecs, ";");
> }
> @@ -3732,7 +3732,8 @@ static void reds_set_video_codecs_from_string(RedsState
> *reds, const char *codec
> }
>
> video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
> -const char *c = codecs;
> +char *codecs_copy = g_strdup_printf("%s;", codecs);
> +char *c = codecs_copy;
> while ( (c = parse_next_video_codec(c, _name, _name)) ) {
> uint32_t encoder_index, codec_index;
> if (!encoder_name || !codec_name) {
> @@ -3755,11 +3756,9 @@ static void
> reds_set_video_codecs_from_string(RedsState *reds, const char *codec
> g_array_append_val(video_codecs, new_codec);
> }
>
> -/* these are allocated by sscanf, do not use g_free */
> -free(encoder_name);
> -free(codec_name);
> codecs = c;
> }
> +g_free(codecs_copy);
>
codecs may refer to free memory after this, I believe.
Would be nice to factor this parsing code out of RedsState and make
some unit tests.
> if (video_codecs->len == 0) {
> spice_warning("Failed to set video codecs, input string: '%s'",
> codecs);
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-common] proto: Remove obsolete TunnelChannel
message {
> -uint32 service_id;
> -TunnelIpInfo virtual_ip;
> -} service_ip_map;
> -
> -message {
> -uint16 connection_id;
> -uint32 service_id;
> -uint32 tokens;
> -} socket_open;
> -
> -message {
> -uint16 connection_id;
> -} socket_fin;
> -
> -message {
> -uint16 connection_id;
> -} socket_close;
> -
> -message {
> -uint16 connection_id;
> -uint8 data[] @end;
> -} socket_data;
> -
> -message {
> -uint16 connection_id;
> -} socket_closed_ack;
> -
> -message {
> -uint16 connection_id;
> -uint32 num_tokens;
> -} @ctype(SpiceMsgTunnelSocketTokens) socket_token;
> -
> - client:
> -message {
> -tunnel_service_type type;
> -uint32 id;
> -uint32 group;
> -uint32 port;
> -uint8 *name[cstring()] @nocopy;
> -uint8 *description[cstring()] @nocopy;
> -switch (type) {
> -case IPP:
> -TunnelIpInfo ip @ctype(SpiceMsgTunnelIpInfo);
> -} u;
> -} @ctype(SpiceMsgcTunnelAddGenericService) service_add = 101;
> -
> -message {
> -uint32 id;
> -} @ctype(SpiceMsgcTunnelRemoveService) service_remove;
> -
> -message {
> -uint16 connection_id;
> -uint32 tokens;
> -} socket_open_ack;
> -
> -message {
> -uint16 connection_id;
> -} socket_open_nack;
> -
> -message {
> -uint16 connection_id;
> -} socket_fin;
> -
> -message {
> -uint16 connection_id;
> -} socket_closed;
> -
> -message {
> -uint16 connection_id;
> -} socket_closed_ack;
> -
> -message {
> -uint16 connection_id;
> -uint8 data[] @end;
> -} socket_data;
> -
> -message {
> -uint16 connection_id;
> -uint32 num_tokens;
> -} @ctype(SpiceMsgcTunnelSocketTokens) socket_token;
> -};
> -
> enum32 vsc_message_type {
> Init = 1,
> Error,
> @@ -1466,8 +1357,8 @@ protocol Spice {
> CursorChannel cursor;
> PlaybackChannel playback;
> RecordChannel record;
> -TunnelChannel tunnel;
> -SmartcardChannel smartcard;
> +// there used to be a TunnelChannel
> +SmartcardChannel smartcard = 8;
> UsbredirChannel usbredir;
> PortChannel port;
> WebDAVChannel webdav;
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2] smartcard: Warn if multiple readers are detected
Hi
On Fri, Feb 22, 2019 at 10:44 AM Frediano Ziglio wrote:
>
> >
> > spice-server does not deal properly with multiple smartcard readers,
> > only the first one will be working. Add a warning when this happens to
> > make it easier to diagnose such issues.
> >
> > Signed-off-by: Christophe Fergeau
> > ---
> >
> > Alternate version using g_list_length rather than manually counting the
> > elements.
> >
> > src/smartcard-manager.c | 19 +++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> > index ceecfdc7..144caf9b 100644
> > --- a/src/smartcard-manager.c
> > +++ b/src/smartcard-manager.c
> > @@ -389,6 +389,24 @@ typedef struct {
> > GError *err;
> > } SmartcardManagerInitArgs;
> >
> > +
> > +static void unref_smartcard_reader(gpointer data)
>
> We usually use verbs after the name, like smartcard_reader_unref.
And in this case, it should probably be smartcard_reader_free()
(otherwise I expect _ref to exist!)
>
> > +{
> > +g_boxed_free(SPICE_TYPE_SMARTCARD_READER, data);
> > +}
> > +
> > +/* spice-server only supports one smartcard reader being in use */
> > +static void smartcard_check_reader_count(void)
> > +{
> > +GList *readers;
> > +
> > +readers =
> > spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
> > +if (g_list_length(readers) > 1) {
> > +g_warning("Multiple smartcard readers are plugged in, only the
> > first
> > one will be shared with the VM");
> > +}
> > +g_list_free_full(readers, unref_smartcard_reader);
> > +}
> > +
> > static gboolean smartcard_manager_init(SmartcardManagerInitArgs *args)
> > {
> > gchar *emul_args = NULL;
> > @@ -442,6 +460,7 @@ init:
> > "Failed to initialize smartcard");
> > goto end;
> > }
> > + smartcard_check_reader_count();
> >
> > retval = TRUE;
> >
>
> I think the ack from Marc-andre is still valid, nothing against it.
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] fixup! display: add -display spice-app launching a Spice client
---
ui/spice-app.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 4f5229f3ee..925b27b708 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -181,6 +181,8 @@ static void spice_app_display_init(DisplayState *ds,
DisplayOptions *opts)
g_app_info_launch_default_for_uri(uri, NULL, );
if (err) {
error_report("Failed to launch %s URI: %s", uri, err->message);
+error_report("You need a capable Spice client, "
+ "such as virt-viewer 8.0");
exit(1);
}
g_free(uri);
--
2.21.0.rc1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk] smartcard: Warn if multiple readers are detected
Hi
On Thu, Feb 21, 2019 at 4:21 PM Christophe Fergeau wrote:
>
> On Thu, Feb 21, 2019 at 03:59:00PM +0100, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Feb 20, 2019 at 5:17 PM Christophe Fergeau
> > wrote:
> > >
> > > spice-server does not deal properly with multiple smartcard readers,
> > > only the first one will be working. Add a warning when this happens to
> > > make it easier to diagnose such issues.
> > >
> > > Signed-off-by: Christophe Fergeau
> > > ---
> > > src/smartcard-manager.c | 20
> > > 1 file changed, 20 insertions(+)
> > >
> > > diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> > > index ceecfdc7..456c3c2e 100644
> > > --- a/src/smartcard-manager.c
> > > +++ b/src/smartcard-manager.c
> > > @@ -389,6 +389,25 @@ typedef struct {
> > > GError *err;
> > > } SmartcardManagerInitArgs;
> > >
> > > +/* spice-server only supports one smartcard reader being in use */
> > > +static void smartcard_check_reader_count(void)
> > > +{
> > > +unsigned int reader_count = 0;
> > > +GList *readers;
> > > +GList *it;
> > > +
> > > +readers =
> > > spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
> > > +
> > > +for (it = readers; it != NULL; it = it->next) {
> > > +reader_count++;
> > > +g_boxed_free(SPICE_TYPE_SMARTCARD_READER, it->data);
> > > +}
> > > +if (reader_count > 1) {
> > > +g_warning("Multiple smartcard readers are plugged in, only the
> > > first one will be shared with the VM");
> > > +}
> > > +g_list_free(readers);
> >
> >
> > looks ok, ack
> >
> > it could eventually be simplified with g_list_count() &
> > g_list_free_full() (that would require wrapping the boxed_free with a
> > helper)
>
> I thought about it, given the need for a wrapper, it felt better to do
> it this way.
>
The wrapper could potentially simplify other parts of the code, your call.
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk] smartcard: Warn if multiple readers are detected
Hi
On Wed, Feb 20, 2019 at 5:17 PM Christophe Fergeau wrote:
>
> spice-server does not deal properly with multiple smartcard readers,
> only the first one will be working. Add a warning when this happens to
> make it easier to diagnose such issues.
>
> Signed-off-by: Christophe Fergeau
> ---
> src/smartcard-manager.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> index ceecfdc7..456c3c2e 100644
> --- a/src/smartcard-manager.c
> +++ b/src/smartcard-manager.c
> @@ -389,6 +389,25 @@ typedef struct {
> GError *err;
> } SmartcardManagerInitArgs;
>
> +/* spice-server only supports one smartcard reader being in use */
> +static void smartcard_check_reader_count(void)
> +{
> +unsigned int reader_count = 0;
> +GList *readers;
> +GList *it;
> +
> +readers =
> spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
> +
> +for (it = readers; it != NULL; it = it->next) {
> +reader_count++;
> +g_boxed_free(SPICE_TYPE_SMARTCARD_READER, it->data);
> +}
> +if (reader_count > 1) {
> +g_warning("Multiple smartcard readers are plugged in, only the first
> one will be shared with the VM");
> +}
> +g_list_free(readers);
looks ok, ack
it could eventually be simplified with g_list_count() &
g_list_free_full() (that would require wrapping the boxed_free with a
helper)
> +}
> +
> static gboolean smartcard_manager_init(SmartcardManagerInitArgs *args)
> {
> gchar *emul_args = NULL;
> @@ -442,6 +461,7 @@ init:
> "Failed to initialize smartcard");
> goto end;
> }
> +smartcard_check_reader_count();
>
> retval = TRUE;
>
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] meson: Pass --msgid-bugs-address to PO generation
Hi
On Tue, Feb 19, 2019 at 6:11 PM Frediano Ziglio wrote:
>
> As autoconf provide e-mail address in the .po files.
> The change in main meson.build is just a move to allow the
> other meson.buuild to see the setting.
>
> Signed-off-by: Frediano Ziglio
ack (pretty useless, even glib/gtk doesn't need it...)
> ---
> meson.build| 38 +++---
> po/meson.build | 1 +
> 2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index c015de7f..1863c52a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -339,6 +339,25 @@ endif
>
> add_project_arguments(compiler.get_supported_arguments(spice_gtk_global_cflags),
>language : 'c')
>
> +#
> +# write config.h
> +#
> +proj_version = meson.project_version()
> +proj_name = meson.project_name()
> +config_data = {'VERSION' : proj_version,
> + 'PACKAGE_VERSION' : proj_version,
> + 'GETTEXT_PACKAGE' : proj_name,
> + 'LOCALE_DIR' : spice_gtk_localedir,
> + 'PACKAGE_STRING' : '@0@ @1@'.format(proj_name, proj_version),
> + 'PACKAGE_BUGREPORT' : 'spice-devel@lists.freedesktop.org'}
> +foreach key, value : config_data
> + spice_gtk_config_data.set_quoted(key, value)
> +endforeach
> +
> +configure_file(output : 'config.h',
> + install : false,
> + configuration : spice_gtk_config_data)
> +
> #
> # Subdirectories
> #
> @@ -359,25 +378,6 @@ subdir('man')
> subdir('po')
> subdir('vapi')
>
> -#
> -# write config.h
> -#
> -proj_version = meson.project_version()
> -proj_name = meson.project_name()
> -config_data = {'VERSION' : proj_version,
> - 'PACKAGE_VERSION' : proj_version,
> - 'GETTEXT_PACKAGE' : proj_name,
> - 'LOCALE_DIR' : spice_gtk_localedir,
> - 'PACKAGE_STRING' : '@0@ @1@'.format(proj_name, proj_version),
> - 'PACKAGE_BUGREPORT' : 'spice-devel@lists.freedesktop.org'}
> -foreach key, value : config_data
> - spice_gtk_config_data.set_quoted(key, value)
> -endforeach
> -
> -configure_file(output : 'config.h',
> - install : false,
> - configuration : spice_gtk_config_data)
> -
> #
> # write spice-client-glib.pc
> #
> diff --git a/po/meson.build b/po/meson.build
> index fb3c3957..da459f2f 100644
> --- a/po/meson.build
> +++ b/po/meson.build
> @@ -1,3 +1,4 @@
> i18n = import('i18n')
> i18n.gettext(meson.project_name(),
> + args : '--msgid-bugs-address=' +
> config_data['PACKAGE_BUGREPORT'],
> preset : 'glib')
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] channel-display-gst: Use recorder for frame statistics
Hi
On Tue, Feb 19, 2019 at 6:02 PM Victor Toso wrote:
>
> Hi,
>
> On Tue, Feb 19, 2019 at 05:37:38PM +0100, Marc-André Lureau wrote:
> > > > > -SPICE_DEBUG("frame mm_time %u size %u creation time %"
> > > > > G_GINT64_FORMAT
> > > > > -" decoded time %" G_GINT64_FORMAT " queue %u",
> > > > > -frame->mm_time, frame->size,
> > > > > frame->creation_time,
> > > > > -duration, decoder->decoding_queue->length);
> > > > > +record(frames_stats,
> > > > > + "frame mm_time %u size %u creation time %" PRId64
> > > > > + " decoded time %" PRId64 " queue %u",
> > > > > + frame->mm_time, frame->size, frame->creation_time,
> > > > > + duration, decoder->decoding_queue->length);
> > > >
> > > > Why SPICE_DEBUG log is removed?
> > > >
> > > > Why is the "recorder" stuff necessary here?
> > > >
> > >
> > > Measurement instrumentation
> >
> > Ok, but can we get have the regular log as well?
>
> This really is measurement type of logs. We don't have structured
And "trace"(?) are better than "log" for "measurements"?
> logging so I do prefer this kind of logging being enabled by some
> dynamic tweaking instead of seeing it all the time.
You can filter it out with grep, fairly easily.
I proposed a series using structured logging and categories for
SPICE_DEBUG= in the past iirc. I guess I should revisit it.
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] channel-display-gst: Use recorder for frame statistics
Hi
On Tue, Feb 19, 2019 at 5:19 PM Frediano Ziglio wrote:
>
> >
> > Hi
> >
> > On Wed, Jan 30, 2019 at 4:00 PM Frediano Ziglio wrote:
> > >
> > > Allows to handle these statistics in a more flexible way.
> > >
> > > Signed-off-by: Frediano Ziglio
> > > ---
> > > src/channel-display-gst.c | 12
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > Changes since v1:
> > > - change formatting constants to system one instead of GLib
> > > to fix build on Windows.
> > >
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index 4272ade8..5bb53b6f 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -20,6 +20,7 @@
> > > #include "spice-client.h"
> > > #include "spice-common.h"
> > > #include "spice-channel-priv.h"
> > > +#include "common/recorder.h"
> > >
> > > #include "channel-display-priv.h"
> > >
> > > @@ -109,6 +110,8 @@ static void schedule_frame(SpiceGstDecoder *decoder);
> > > static void fetch_pending_sample(SpiceGstDecoder *decoder);
> > > static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder,
> > > GstBuffer *buffer);
> > >
> > > +RECORDER(frames_stats, 64, "Frames statistics");
> > > +
> >
> > What is this 64 value for?
> >
>
> It's the ring size.
Ok, but why 64?
>
> > > static int spice_gst_buffer_get_stride(GstBuffer *buffer)
> > > {
> > > GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
> > > @@ -248,10 +251,11 @@ static SpiceGstFrame
> > > *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer *buf
> > >
> > > const SpiceFrame *frame = gstframe->encoded_frame;
> > > int64_t duration = g_get_monotonic_time() - frame->creation_time;
> > > -SPICE_DEBUG("frame mm_time %u size %u creation time %"
> > > G_GINT64_FORMAT
> > > -" decoded time %" G_GINT64_FORMAT " queue %u",
> > > -frame->mm_time, frame->size, frame->creation_time,
> > > -duration, decoder->decoding_queue->length);
> > > +record(frames_stats,
> > > + "frame mm_time %u size %u creation time %" PRId64
> > > + " decoded time %" PRId64 " queue %u",
> > > + frame->mm_time, frame->size, frame->creation_time,
> > > + duration, decoder->decoding_queue->length);
> >
> > Why SPICE_DEBUG log is removed?
> >
> > Why is the "recorder" stuff necessary here?
> >
>
> Measurement instrumentation
Ok, but can we get have the regular log as well?
>
> > > }
> > > return gstframe;
> > > }
>
> Frediano
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] channel-display-gst: Use recorder for frame statistics
Hi
On Wed, Jan 30, 2019 at 4:00 PM Frediano Ziglio wrote:
>
> Allows to handle these statistics in a more flexible way.
>
> Signed-off-by: Frediano Ziglio
> ---
> src/channel-display-gst.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> Changes since v1:
> - change formatting constants to system one instead of GLib
> to fix build on Windows.
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 4272ade8..5bb53b6f 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -20,6 +20,7 @@
> #include "spice-client.h"
> #include "spice-common.h"
> #include "spice-channel-priv.h"
> +#include "common/recorder.h"
>
> #include "channel-display-priv.h"
>
> @@ -109,6 +110,8 @@ static void schedule_frame(SpiceGstDecoder *decoder);
> static void fetch_pending_sample(SpiceGstDecoder *decoder);
> static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
> *buffer);
>
> +RECORDER(frames_stats, 64, "Frames statistics");
> +
What is this 64 value for?
> static int spice_gst_buffer_get_stride(GstBuffer *buffer)
> {
> GstVideoMeta *video = gst_buffer_get_video_meta(buffer);
> @@ -248,10 +251,11 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
> *decoder, GstBuffer *buf
>
> const SpiceFrame *frame = gstframe->encoded_frame;
> int64_t duration = g_get_monotonic_time() - frame->creation_time;
> -SPICE_DEBUG("frame mm_time %u size %u creation time %"
> G_GINT64_FORMAT
> -" decoded time %" G_GINT64_FORMAT " queue %u",
> -frame->mm_time, frame->size, frame->creation_time,
> -duration, decoder->decoding_queue->length);
> +record(frames_stats,
> + "frame mm_time %u size %u creation time %" PRId64
> + " decoded time %" PRId64 " queue %u",
> + frame->mm_time, frame->size, frame->creation_time,
> + duration, decoder->decoding_queue->length);
Why SPICE_DEBUG log is removed?
Why is the "recorder" stuff necessary here?
> }
> return gstframe;
> }
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] Move src/keycodemapdb -> subprojects/keycodemapdb
Hi On Fri, Feb 15, 2019 at 8:49 PM Marc-André Lureau wrote: > > Hi > > On Fri, Feb 15, 2019 at 6:21 PM Frediano Ziglio wrote: > > Looking at http://mesonbuild.com/Subprojects.html looks like > > subprojects should be Meson project too. > > While spice-common is now a Meson project keycodemapdb is not > > so it does not seem that great to declare it as subproject. > > Ok, it's a bit silly imho. Let's make one then: > https://gitlab.com/keycodemap/keycodemapdb/merge_requests/7 It's applied now, ack with the subproject update? > > -- > Marc-André Lureau -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] Move src/keycodemapdb -> subprojects/keycodemapdb
Hi On Fri, Feb 15, 2019 at 6:21 PM Frediano Ziglio wrote: > Looking at http://mesonbuild.com/Subprojects.html looks like > subprojects should be Meson project too. > While spice-common is now a Meson project keycodemapdb is not > so it does not seem that great to declare it as subproject. Ok, it's a bit silly imho. Let's make one then: https://gitlab.com/keycodemap/keycodemapdb/merge_requests/7 -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] Move src/keycodemapdb -> subprojects/keycodemapdb
Hi
On Fri, Feb 15, 2019 at 5:21 PM Frediano Ziglio wrote:
>
> > Hi
> >
> > On Fri, Feb 15, 2019 at 4:04 PM Frediano Ziglio wrote:
> > >
> > > >
> > > > From: Marc-André Lureau
> > > >
> > > > Follow meson build system conventions.
> > > >
> > > > This will allow meson to handle it as a subproject.
> > > >
> > > > Signed-off-by: Marc-André Lureau
> > > > ---
> > > > Changes since v1:
> > > > - rebase;
> > > > - support still Autoconf.
> > > > ---
> > > > .gitmodules | 4 ++--
> > > > meson.build | 6 +-
> > > > src/Makefile.am | 20 ++--
> > > > src/meson.build | 2 --
> > > > {src => subprojects}/keycodemapdb | 0
> > > > 5 files changed, 17 insertions(+), 15 deletions(-)
> > > > rename {src => subprojects}/keycodemapdb (100%)
> > > >
> > > > diff --git a/.gitmodules b/.gitmodules
> > > > index 6938cd0c..a7804e6f 100644
> > > > --- a/.gitmodules
> > > > +++ b/.gitmodules
> > > > @@ -1,6 +1,6 @@
> > > > [submodule "spice-common"]
> > > > path = subprojects/spice-common
> > > > url = ../spice-common.git
> > > > -[submodule "src/keycodemapdb"]
> > > > - path = src/keycodemapdb
> > > > +[submodule "subprojects/keycodemapdb"]
>
> Why don't we call it just "keycodemapdb", is not necessary to
> follow the path (also would be coherent with "spice-common").
Tbh, I don't care.
>
> > > > + path = subprojects/keycodemapdb
> > > > url = https://gitlab.com/keycodemap/keycodemapdb.git
> > > > diff --git a/meson.build b/meson.build
> > > > index 1276fb95..9fa94fc4 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -38,12 +38,16 @@ spice_gtk_deps = []
> > > > spice_acl_deps = []
> > > >
> > > > #
> > > > -# Spice common subproject
> > > > +# Set up subprojects
> > > > #
> > > > spice_common = subproject('spice-common', default_options :
> > > > ['generate-code=client'])
> > > >
> > > > spice_gtk_config_data.merge_from(spice_common.get_variable('spice_common_config_data'))
> > > > spice_glib_deps += spice_common.get_variable('spice_common_client_dep')
> > > >
> > > > +subproject('keycodemapdb', required : false)
> > >
> > > Why required is false? I don't think spice-gtk will compile
> > > without it.
> >
> > I don't remember adding that, please remove if you commit.
> >
>
> Not a nice idea. Meson is expecting a meson.build inside it and
> fails.
Ah that rings a bell :)
thanks for checking
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 08/12] meson: remove our own submodule update
Hi
On Fri, Feb 15, 2019 at 5:19 PM Frediano Ziglio wrote:
>
> > Hi
> >
> > On Fri, Feb 15, 2019 at 3:57 PM Frediano Ziglio wrote:
> > >
> > > > From: Marc-André Lureau
> > > >
> > > > Our own handling was limited, since it checked only spice-common.
> > > >
> > > > This is handled by meson since v0.40 for subprojects.
> > > >
> > > > Signed-off-by: Marc-André Lureau
> > > > ---
> > > > build-aux/meson/check-spice-common | 5 -
> > > > meson.build| 3 ---
> > > > 2 files changed, 8 deletions(-)
> > > > delete mode 100755 build-aux/meson/check-spice-common
> > > >
> > > > diff --git a/build-aux/meson/check-spice-common
> > > > b/build-aux/meson/check-spice-common
> > > > deleted file mode 100755
> > > > index a0d03a6..000
> > > > --- a/build-aux/meson/check-spice-common
> > > > +++ /dev/null
> > > > @@ -1,5 +0,0 @@
> > > > -#!/bin/sh
> > > > -set -e
> > > > -if git ls-files --error-unmatch
> > > > ${MESON_SOURCE_ROOT}/subprojects/spice-common > /dev/null 2>&1; then
> > > > -git --git-dir="${MESON_SOURCE_ROOT}/.git" submodule update --init
> > > > --recursive
> > > > -fi
> > > > diff --git a/meson.build b/meson.build
> > > > index 29d5fed..c5dc438 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -6,9 +6,6 @@ project('spice-gtk', 'c',
> > > > license : 'LGPLv2.1',
> > > > meson_version : '>= 0.49')
> > > >
> > > > -message('Updating submodules')
> > > > -run_command('build-aux/meson/check-spice-common', check : true)
> > > > -
> > > > #
> > > > # global C defines
> > > > #
> > >
> > > Without this patch Meson does not sync the submodules automatically.
> >
> >
> > Hmm weird, what error do you get?
> >
> > (obviously it requires "Move src/keycodemapdb ->
> > subprojects/keycodemapdb" before)
> >
>
> If I checkout an old spice-common it does not update it and
> give error that cannot find meson.build.
I don't understand what you are trying to do.
All the build-sys should do wrt submodule handling is initial checkout.
If you modify the submodule later, it shouldn't re-sync it on configure/build.
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] Move src/keycodemapdb -> subprojects/keycodemapdb
On Fri, Feb 15, 2019 at 4:54 PM Frediano Ziglio wrote:
>
> > Hi
> >
> > On Fri, Feb 15, 2019 at 4:04 PM Frediano Ziglio wrote:
> > >
> > > >
> > > > From: Marc-André Lureau
> > > >
> > > > Follow meson build system conventions.
> > > >
> > > > This will allow meson to handle it as a subproject.
> > > >
> > > > Signed-off-by: Marc-André Lureau
> > > > ---
> > > > Changes since v1:
> > > > - rebase;
> > > > - support still Autoconf.
> > > > ---
> > > > .gitmodules | 4 ++--
> > > > meson.build | 6 +-
> > > > src/Makefile.am | 20 ++--
> > > > src/meson.build | 2 --
> > > > {src => subprojects}/keycodemapdb | 0
> > > > 5 files changed, 17 insertions(+), 15 deletions(-)
> > > > rename {src => subprojects}/keycodemapdb (100%)
> > > >
> > > > diff --git a/.gitmodules b/.gitmodules
> > > > index 6938cd0c..a7804e6f 100644
> > > > --- a/.gitmodules
> > > > +++ b/.gitmodules
> > > > @@ -1,6 +1,6 @@
> > > > [submodule "spice-common"]
> > > > path = subprojects/spice-common
> > > > url = ../spice-common.git
> > > > -[submodule "src/keycodemapdb"]
> > > > - path = src/keycodemapdb
> > > > +[submodule "subprojects/keycodemapdb"]
> > > > + path = subprojects/keycodemapdb
> > > > url = https://gitlab.com/keycodemap/keycodemapdb.git
> > > > diff --git a/meson.build b/meson.build
> > > > index 1276fb95..9fa94fc4 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -38,12 +38,16 @@ spice_gtk_deps = []
> > > > spice_acl_deps = []
> > > >
> > > > #
> > > > -# Spice common subproject
> > > > +# Set up subprojects
> > > > #
> > > > spice_common = subproject('spice-common', default_options :
> > > > ['generate-code=client'])
> > > >
> > > > spice_gtk_config_data.merge_from(spice_common.get_variable('spice_common_config_data'))
> > > > spice_glib_deps += spice_common.get_variable('spice_common_client_dep')
> > > >
> > > > +subproject('keycodemapdb', required : false)
> > >
> > > Why required is false? I don't think spice-gtk will compile
> > > without it.
> >
> > I don't remember adding that, please remove if you commit.
> >
>
> I can do it. Are you fine with the Autoconf addition? Do you ack?
ack
thanks
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 05/12] meson: add vcs_tag to session init debug log
Hi
On Fri, Feb 15, 2019 at 3:57 PM Frediano Ziglio wrote:
>
> >
> > From: Marc-André Lureau
> >
> > Use the trick recommended here to generate a vcs_tag.h at build-time:
> > https://github.com/mesonbuild/meson/issues/3903
> >
> > Hopefully, meson will learn to generate project version from git:
> > https://github.com/mesonbuild/meson/issues/688
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/meson.build | 12
> > src/spice-session.c | 3 ++-
> > src/vcs_tag.h.in| 1 +
> > 3 files changed, 15 insertions(+), 1 deletion(-)
> > create mode 100644 src/vcs_tag.h.in
> >
> > diff --git a/src/meson.build b/src/meson.build
> > index c55db44..e580429 100644
> > --- a/src/meson.build
> > +++ b/src/meson.build
> > @@ -82,9 +82,21 @@ spice_client_glib_introspection_sources = [
> >'usb-device-manager.c',
> > ]
> >
> > +vcs_conf = configuration_data()
> > +vcs_conf.set('VCS_TAG', '@VCS_TAG@')
> > +vcs_tag_h = vcs_tag(
> > + input : configure_file(
> > +input : 'vcs_tag.h.in',
> > +output : 'vcs_tag.h.in',
> > +configuration: vcs_conf,
> > + ),
> > + output : 'vcs_tag.h',
> > +)
> > +
> > spice_client_glib_sources = [
> >spice_marshals,
> >spice_client_glib_introspection_sources,
> > + vcs_tag_h,
> >'bio-gio.c',
> >'bio-gio.h',
> >'channel-base.c',
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index 5609c9b..d9825e3 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -32,6 +32,7 @@
> > #include "spice-uri-priv.h"
> > #include "channel-playback-priv.h"
> > #include "spice-audio-priv.h"
> > +#include "vcs_tag.h"
> >
> > #define IMAGES_CACHE_SIZE_DEFAULT (1024 * 1024 * 80)
> > #define MIN_GLZ_WINDOW_SIZE_DEFAULT (1024 * 1024 * 12)
> > @@ -277,7 +278,7 @@ static void spice_session_init(SpiceSession *session)
> > SpiceSessionPrivate *s;
> > gchar *channels;
> >
> > -SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
> > +SPICE_DEBUG("New session (package version: " VCS_TAG ")");
> > s = session->priv = spice_session_get_instance_private(session);
> >
> > channels = spice_channel_supported_string();
>
> Why is this patch needed? Meson already generates PACKAGE_STRING in config.h
>
It is no longer needed now that you made git-version-gen work again
(so the git version is included in PACKAGE_STRING)
We might need to revisit this patch depending on how meson finally solves !688
> > diff --git a/src/vcs_tag.h.in b/src/vcs_tag.h.in
> > new file mode 100644
> > index 000..5d8985d
> > --- /dev/null
> > +++ b/src/vcs_tag.h.in
> > @@ -0,0 +1 @@
> > +#define VCS_TAG "@VCS_TAG@"
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] Move src/keycodemapdb -> subprojects/keycodemapdb
Hi
On Fri, Feb 15, 2019 at 4:04 PM Frediano Ziglio wrote:
>
> >
> > From: Marc-André Lureau
> >
> > Follow meson build system conventions.
> >
> > This will allow meson to handle it as a subproject.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > Changes since v1:
> > - rebase;
> > - support still Autoconf.
> > ---
> > .gitmodules | 4 ++--
> > meson.build | 6 +-
> > src/Makefile.am | 20 ++--
> > src/meson.build | 2 --
> > {src => subprojects}/keycodemapdb | 0
> > 5 files changed, 17 insertions(+), 15 deletions(-)
> > rename {src => subprojects}/keycodemapdb (100%)
> >
> > diff --git a/.gitmodules b/.gitmodules
> > index 6938cd0c..a7804e6f 100644
> > --- a/.gitmodules
> > +++ b/.gitmodules
> > @@ -1,6 +1,6 @@
> > [submodule "spice-common"]
> > path = subprojects/spice-common
> > url = ../spice-common.git
> > -[submodule "src/keycodemapdb"]
> > - path = src/keycodemapdb
> > +[submodule "subprojects/keycodemapdb"]
> > + path = subprojects/keycodemapdb
> > url = https://gitlab.com/keycodemap/keycodemapdb.git
> > diff --git a/meson.build b/meson.build
> > index 1276fb95..9fa94fc4 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -38,12 +38,16 @@ spice_gtk_deps = []
> > spice_acl_deps = []
> >
> > #
> > -# Spice common subproject
> > +# Set up subprojects
> > #
> > spice_common = subproject('spice-common', default_options :
> > ['generate-code=client'])
> >
> > spice_gtk_config_data.merge_from(spice_common.get_variable('spice_common_config_data'))
> > spice_glib_deps += spice_common.get_variable('spice_common_client_dep')
> >
> > +subproject('keycodemapdb', required : false)
>
> Why required is false? I don't think spice-gtk will compile
> without it.
I don't remember adding that, please remove if you commit.
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 08/12] meson: remove our own submodule update
Hi
On Fri, Feb 15, 2019 at 3:57 PM Frediano Ziglio wrote:
>
> > From: Marc-André Lureau
> >
> > Our own handling was limited, since it checked only spice-common.
> >
> > This is handled by meson since v0.40 for subprojects.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > build-aux/meson/check-spice-common | 5 -
> > meson.build| 3 ---
> > 2 files changed, 8 deletions(-)
> > delete mode 100755 build-aux/meson/check-spice-common
> >
> > diff --git a/build-aux/meson/check-spice-common
> > b/build-aux/meson/check-spice-common
> > deleted file mode 100755
> > index a0d03a6..000
> > --- a/build-aux/meson/check-spice-common
> > +++ /dev/null
> > @@ -1,5 +0,0 @@
> > -#!/bin/sh
> > -set -e
> > -if git ls-files --error-unmatch
> > ${MESON_SOURCE_ROOT}/subprojects/spice-common > /dev/null 2>&1; then
> > -git --git-dir="${MESON_SOURCE_ROOT}/.git" submodule update --init
> > --recursive
> > -fi
> > diff --git a/meson.build b/meson.build
> > index 29d5fed..c5dc438 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -6,9 +6,6 @@ project('spice-gtk', 'c',
> > license : 'LGPLv2.1',
> > meson_version : '>= 0.49')
> >
> > -message('Updating submodules')
> > -run_command('build-aux/meson/check-spice-common', check : true)
> > -
> > #
> > # global C defines
> > #
>
> Without this patch Meson does not sync the submodules automatically.
Hmm weird, what error do you get?
(obviously it requires "Move src/keycodemapdb ->
subprojects/keycodemapdb" before)
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk] desktop-integration: Avoid empty private structure
On Fri, Feb 15, 2019 at 3:57 PM Frediano Ziglio wrote:
>
> This causes a critical warning as private structures should not
> be empty.
>
> Signed-off-by: Frediano Ziglio
ack
> ---
> src/desktop-integration.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/desktop-integration.c b/src/desktop-integration.c
> index 8458efaa..6f569ae2 100644
> --- a/src/desktop-integration.c
> +++ b/src/desktop-integration.c
> @@ -38,6 +38,9 @@ struct _SpiceDesktopIntegrationPrivate {
> #ifdef WITH_GNOME
> GDBusProxy *gnome_session_proxy;
> guint gnome_automount_inhibit_cookie;
> +#else
> +/* private structures cannot be empty in GLib */
> +int dummy;
> #endif
> };
>
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 03/12] meson: fix po generation
Hi
On Tue, Feb 12, 2019 at 12:35 PM Frediano Ziglio wrote:
>
>
> > From: Marc-André Lureau
> >
> > Use glib preset (from meson v0.37) to catch all our translatable
> > strings and use good default settings.
> >
> > While at it, remove the needless directory argument.
> >
> > Signed-off-by: Marc-André Lureau
>
> With or without this patch Meson seems to do nothing in the po/
> directory, .gmo files are not generated.
> I cannot see any warning in Meson output about i18n.
>
For some reasons, meson doesn't build everything at build time by
default, some things are delayed at install time (such as doc).
Try:
ninja meson-spice-gtk-gmo
> Frediano
>
> > ---
> > po/meson.build | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/po/meson.build b/po/meson.build
> > index 60c27a7..fb3c395 100644
> > --- a/po/meson.build
> > +++ b/po/meson.build
> > @@ -1,3 +1,3 @@
> > i18n = import('i18n')
> > i18n.gettext(meson.project_name(),
> > - args : '--directory=@0@'.format(meson.source_root()))
> > + preset : 'glib')
> ___________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 01/12] meson: improve gtk-doc build
Hi
On Mon, Feb 11, 2019 at 6:16 PM Frediano Ziglio wrote:
>
> > >
> > > From: Marc-André Lureau
> > >
> > > - Fix the following warnings:
> > > ./spice-gtk-sections.txt:467: warning: No declaration found for
> > > SPICE_GTK_CHECK_VERSION.
> > > ./spice-gtk-sections.txt:468: warning: No declaration found for
> > > SPICE_GTK_MAJOR_VERSION.
> > > ./spice-gtk-sections.txt:469: warning: No declaration found for
> > > SPICE_GTK_MICRO_VERSION.
> > > ./spice-gtk-sections.txt:470: warning: No declaration found for
> > > SPICE_GTK_MINOR_VERSION.
> > >
> > > - fixxref for glib and gtk (thus requires gtk+ to build doc)
> > >
> > > - And other minor simplifications.
> > >
> > > After autotools is removed, we should try to use --rebuild-types. For
> > > now I prefer not to touch it :)
> > >
> >
> > This last sentence is not related to this commit.
Kind of related, since I wanted to use this as part of gtkdoc build
improvements. It's a soft FIXME reminder.
> >
> > > Signed-off-by: Marc-André Lureau
> > > ---
> > > doc/reference/meson.build | 29 +++--
> > > 1 file changed, 23 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/doc/reference/meson.build b/doc/reference/meson.build
> > > index a121e66..c1abeb9 100644
> > > --- a/doc/reference/meson.build
> > > +++ b/doc/reference/meson.build
> > > @@ -38,14 +38,31 @@ ignore_headers = [
> > >
> > > spice_gtk_doc_dep = declare_dependency(link_with : [spice_client_gtk_lib,
> > > spice_client_glib_lib])
> > >
> > > -gnome.gtkdoc('spice-gtk',
> > > - content_files : ['spice-gtk-overrides.txt',
> > > 'spice-gtk-overrides.txt'],
> > > +glib_prefix = dependency('glib-2.0').get_pkgconfig_variable('prefix')
> > > +glib_docpath = join_paths(glib_prefix, 'share', 'gtk-doc', 'html')
> > > +gtk_prefix = dependency('gtk+-3.0').get_pkgconfig_variable('prefix')
> > > +gtk_docpath = join_paths(gtk_prefix, 'share', 'gtk-doc', 'html')
> > > +docpath = join_paths(spice_gtk_datadir, 'gtk-doc', 'html')
> > > +
> > > +gnome.gtkdoc(meson.project_name(),
> > > dependencies : spice_gtk_doc_dep,
> > > - main_xml : 'spice-gtk-docs.xml',
> > > - gobject_typesfile : files('spice-gtk.types'),
> > > + main_xml : meson.project_name() + '-docs.xml',
> > > + gobject_typesfile : meson.project_name() + '.types',
> > > ignore_headers : ignore_headers,
> > > include_directories: spice_gtk_include,
> > > c_args : '-DSPICE_COMPILATION',
> > > install : true,
> > > - scan_args :
> > > ['--deprecated-guards="SPICE_DISABLE_DEPRECATED"',
> > > '--ignore-decorators="G_GNUC_INTERNAL"'],
> > > - src_dir : join_paths(meson.source_root(), 'src'))
> > > + scan_args : [
> > > + '--deprecated-guards="SPICE_DISABLE_DEPRECATED"',
> > > + '--ignore-decorators="G_GNUC_INTERNAL"',
> > > + join_paths(meson.build_root(), 'src', 'spice-version.h')
>
> Why do you need to pass spice-version.h? It's not passed in autoconf files.
> Looks like autoconf is adding all src directory but ignoring lot of files.
> But autoconf does not need to include explicitly spice-version.h
It fixes
./spice-gtk-sections.txt:467: warning: No declaration found for
SPICE_GTK_CHECK_VERSION.
./spice-gtk-sections.txt:468: warning: No declaration found for
SPICE_GTK_MAJOR_VERSION.
./spice-gtk-sections.txt:469: warning: No declaration found for
SPICE_GTK_MICRO_VERSION.
./spice-gtk-sections.txt:470: warning: No declaration found for
SPICE_GTK_MINOR_VERSION.
I have no interest in fixing both autotools & meson, as I want to drop
autotools asap.
This is not critical or a regression, feel free to fix autotools too
>
> > > + ],
> > > + src_dir : join_paths(meson.source_root(), 'src'),
> >
> > These last lines were changed since previous version.
> > What's the reason?
> >
> > > + fixxref_args: [
> > > + '--html-dir=@0@'.format(docpath),
> > > + '--extra-dir=@0@'.format(join_paths(glib_docpath,
> > > 'glib')),
> > > + '--extra-dir=@0@'.format(join_paths(glib_docpath,
> > > 'gobject')),
> > > + '--extra-dir=@0
Re: [Spice-devel] [spice-gtk v2 1/5] gitlab-ci: group and rename jobs
On Tue, Feb 12, 2019 at 3:18 PM Victor Toso wrote:
>
> From: Victor Toso
>
> Group by target build instead of command. The focus of each job is to
> check any regression for given platform, using 'fedora'/'windows' and
> 'autotools'/'meson' seems more intuitive.
>
> By doing that we are grouping similar jobs together, this is
> intentional as we are reducing the amount of jobs that need to be run
> (together with the whole bootstrapping) without losing logs or tests.
>
> Some indentation takes place too, keeping it to 2 spaces as some other
> places in the code
>
> Signed-off-by: Victor Toso
ack
> ---
> .gitlab-ci.yml | 62 --
> 1 file changed, 30 insertions(+), 32 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 74280e9..aab5f77 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -16,41 +16,39 @@ before_script:
>- git clone ${CI_REPOSITORY_URL/spice-gtk/spice-protocol}
>- (cd spice-protocol && ./autogen.sh --prefix=/usr && make install)
>
> -makecheck:
> +fedora-autotools:
>script:
> - - ./autogen.sh --enable-static
> - - make -j4
> - - make check
> +# Run with default options
> +- ./autogen.sh --enable-static
> +- make -j4
> +- make check
> +# Run without features
> +- git clean -xfd
> +- ./autogen.sh --enable-static
> + --enable-lz4=no
> + --enable-webdav=no
> + --with-sasl=no
> + --with-coroutine=auto
> + --enable-pulse=no
> + --enable-smartcard=no
> + --enable-usbredir=no
> +- make -j4
> +- make check
>
> -makecheck-meson:
> +fedora-meson:
>script:
> - - meson build || (cat build/meson-logs/meson-log.txt && exit 1)
> - - ninja -C build
> - - (cd build && meson test) || (cat build/meson-logs/testlog.txt && exit 1)
> +- meson _build_default || (cat _build_default/meson-logs/meson-log.txt
> && exit 1)
> +- ninja -C _build_default
> +- ninja -C _build_default test || (cat
> _build_default/meson-logs/testlog.txt && exit 1)
>
> -makecheck_simple:
> - script:
> - - ./autogen.sh --enable-static
> ---enable-lz4=no
> ---enable-webdav=no
> ---with-sasl=no
> ---with-coroutine=auto
> ---enable-pulse=no
> ---enable-smartcard=no
> ---enable-usbredir=no
> - - make -j4
> - - make check
> -
> -makecheck_simple-meson:
> - script:
> - - meson build -Dauto_features=disabled || (cat
> build/meson-logs/meson-log.txt && exit 1)
> - - ninja -C build
> - - (cd build && meson test) || (cat build/meson-logs/testlog.txt && exit 1)
> +- meson _build_feat_disabled -Dauto_features=disabled || (cat
> _build_feat_disabled/meson-logs/meson-log.txt && exit 1)
> +- ninja -C _build_feat_disabled
> +- ninja -C _build_feat_disabled test || (cat
> _build_feat_disabled/meson-logs/testlog.txt && exit 1)
>
> -make-win:
> +windows-autotools:
>script:
> - - dnf install -y mingw64-gcc mingw64-pkg-config mingw64-pixman
> mingw64-openssl mingw64-gtk3 mingw64-json-glib mingw64-opus
> mingw64-gstreamer1-plugins-base mingw64-gstreamer1-plugins-good
> - - (cd spice-protocol && make clean && mingw64-configure --prefix=/usr &&
> make install)
> - - NOCONFIGURE=yes ./autogen.sh
> - - PYTHON=python3 mingw64-configure --enable-static
> - - make -j4
> +- dnf install -y mingw64-gcc mingw64-pkg-config mingw64-pixman
> mingw64-openssl mingw64-gtk3 mingw64-json-glib mingw64-opus
> mingw64-gstreamer1-plugins-base mingw64-gstreamer1-plugins-good
> +- (cd spice-protocol && make clean && mingw64-configure --prefix=/usr &&
> make install)
> +- NOCONFIGURE=yes ./autogen.sh
> +- PYTHON=python3 mingw64-configure --enable-static
> +- make -j4
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1] Remove spicy flatpak from spice-gtk source code
On Tue, Feb 12, 2019 at 3:01 PM Victor Toso wrote:
>
> From: Victor Toso
>
> The usage of testing tool as flatkpak is discouraged by upstream
> developers, see comments from thread:
>
>
> https://lists.freedesktop.org/archives/spice-devel/2019-February/047877.html
>
> One might argue that keep this might be useful for documentation. For
> that, please refer to updated documentation from Flatpak and
> applications that have adopted Flatpak integration such as GNOME
> Boxes.
>
> Signed-off-by: Victor Toso
ack
> ---
> data/org.spicespace.spicy.json | 71 --
> 1 file changed, 71 deletions(-)
> delete mode 100644 data/org.spicespace.spicy.json
>
> diff --git a/data/org.spicespace.spicy.json b/data/org.spicespace.spicy.json
> deleted file mode 100644
> index 5b0dfbf..000
> --- a/data/org.spicespace.spicy.json
> +++ /dev/null
> @@ -1,71 +0,0 @@
> -{
> -"app-id": "org.spicespace.spicy",
> -"runtime": "org.gnome.Platform",
> -"runtime-version": "3.24",
> -"sdk": "org.gnome.Sdk",
> -"command": "spicy",
> -"tags": ["devel", "development", "nightly"],
> -"finish-args": [
> -"--share=ipc", "--socket=x11",
> -"--socket=wayland",
> -"--socket=pulseaudio",
> -"--share=network"
> -],
> -"modules": [
> -{
> -"name": "python-six",
> -"buildsystem": "simple",
> -"build-commands": [
> -"pip3 install --target=/app six-1.10.0-py2.py3-none-any.whl"
> -],
> -"sources": [
> -{
> - "type": "file",
> - "url":
> "https://pypi.python.org/packages/c8/0a/b6723e1bc4c516cb687841499455a8505b44607ab535be01091c0f24f079/six-1.10.0-py2.py3-none-any.whl#md5=3ab558cf5d4f7a72611d59a81a315dc8;,
> - "sha256":
> "0ff78c403d9bccf5a425a6d31a12aa6b47f1c21ca4dc2573a7e2f32a97335eb1"
> -}
> -]
> -},
> -{
> -"name": "pyparsing",
> -"buildsystem": "simple",
> -"build-commands": [
> -"pip3 install --target=/app
> pyparsing-2.0.3-py2.py3-none-any.whl"
> -],
> -"sources": [
> -{
> -"type": "file",
> -"url":
> "https://pypi.python.org/packages/8f/f4/3a70b5e5b865b1ec45fe48dc5a57cd4facb5c7bd80e5cb1255c362732e81/pyparsing-2.0.3-py2.py3-none-any.whl#md5=4c16ef222caea2e5de741ae503aae652;,
> -"sha256":
> "a9c896bab06dbf3759ad5fb63cfdb3777191e2c4ae640e6dd69ed37530f6ba56"
> -}
> -]
> -},
> -{
> -"builddir": true,
> -"name": "spice-protocol",
> -"sources": [
> -{
> -"type": "git",
> -"url":
> "https://gitlab.freedesktop.org/spice/spice-protocol.git;
> -}
> -]
> -},
> - {
> - "builddir": true,
> -"name": "spice-gtk",
> -"build-options": {
> -"env": {
> -"PYTHONPATH": "/app"
> -}
> -},
> -"config-opts": [ "--disable-vala",
> - "--disable-gtk-doc", "--enable-python-checks" ],
> -"sources": [
> -{
> -"type": "git",
> -"url":
> "https://gitlab.freedesktop.org/spice/spice-gtk.git;
> -}
> -]
> -}
> -]
> -}
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1 00/10] Flatpak + CI
Hi On Mon, Feb 11, 2019 at 6:12 PM Christophe Fergeau wrote: > I think the main objection is with making spicy too easy to install (and > to upgrade). Once we ask someone to test a spicy flatpak and it works > for them, we don't want them to stick to it, start requesting for > flathub availability so that it gets regularly updated, and for this one > small feature that would make spicy a perfect fit for them (which is why > in the first place Marc-André has been trying to discourage use of > spicy). > Indeed. So far it is there as an "example": commit 64a0eeab8ddd2ca6b2d3b57b7f46e99877bfab7e Author: Pavel Grunt Date: Fri Jul 21 11:02:57 2017 +0200 Add flatpak builder manifest file for spicy To give an example for creating flatpaks depending on spice-gtk Tbh, I think we should remove the flatpak from spice-gtk source tree. It doesn't make much sense to have it included imho, unless we have a good reason to build it on a regular basis, which imho is not something we need as a library or even a testing client. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1 00/10] Flatpak + CI
Hi On Mon, Feb 11, 2019 at 2:57 PM Victor Toso wrote: > > On Mon, Feb 11, 2019 at 02:44:23PM +0100, Marc-André Lureau wrote: > > Hi > > > > On Mon, Feb 11, 2019 at 2:21 PM Victor Toso wrote: > > > > > > Hi, > > > > > > On Mon, Feb 11, 2019 at 02:06:19PM +0100, Marc-André Lureau wrote: > > > > Hi > > > > > > > > On Mon, Feb 11, 2019 at 1:59 PM Victor Toso > > > > wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Mon, Feb 11, 2019 at 01:09:41PM +0100, Marc-André Lureau wrote: > > > > > > > Having a virt-viewer flatpak does not mean _not_ having a > > > > > > > spicy.flatkpak (to me); one is full featured spice client while > > > > > > > the other a testing tool... > > > > > > > > > > > > It looks like a lot of duplication of flatpak effort. Maybe you > > > > > > could > > > > > > simply ship spicy in virt-viewer flatpak, so it could be run from > > > > > > command line (please no .desktop) > > > > > > > > > > Is the fact that we are installing a .desktop for spicy the only > > > > > issue here or you don't want to see a flatpak of spicy in the > > > > > gitlab-ci anyway? (btw, I'm not planning to upload this to any > > > > > flatpak provider). > > > > > > > > What's the point in building a spice-gtk flatpak then, when you > > > > have virt-viewer flatpak? > > > > > > You replied my question with another question. > > > > > > My main motivation is that spicy is self contained in spice-gtk, > > > smaller and targeted to be a testing tool, so, testing spice-gtk > > > changes. > > > > I see, you would like CI build version readily available. > > As you can see by browsing the link in the cover letter > > > https://gitlab.freedesktop.org/victortoso/spice-gtk/-/jobs/105184/artifacts/browse > > > (kind of a waste of space to me, but it may be useful) > > 1.1 MB. To avoid waste of space, the flatkpak is not generated > automatically but manually, that is, you have to click on this > 'job' in the CI to activate it; besides that, the artifact is set > to expire_in: 2 days Ok, I don't see much point in having it built in the CI then > > > virt-viewer with only --enable-spice-gtk shouldn't have much more > > dependencies though. > > > > If I add virt-viewer -> flatpak or msi installer to gitlab-ci's > > > artifacts, that's out of scope of spice-gtk although I'll be > > > using it all the time... > > > > Oh you are thinking about building virt-viewer from spice-gtk CI? > > interesting... I wonder if there are mechanisms already to trigger > > rebuilds of dependent projects, I am pretty sure there are > > solutions to that. And flatpak build can pull from upstream > > repository master I guess. > > My interest is testing spice/spice-gtk only. So, to regenerate > virt-viewer flatpak from spice-gtk CI because there is a new > commit in virt-viewer is totally out of context, for me. No, but your interest seems to have a flatpak readily available when doing a commit in spice-gtk. > > I'd be glad if virt-viewer was in gitlab, close to no knowledge > around Pagure infra. > > I still don't know if your earlier concern around .desktop is due > the fact we are installing it (patch 04/10) or with spicy > flatkpak itself. I am concern about distributing spice-gtk and spicy in new forms in general. The .desktop is pretty much a no-go to me. The flatpak I don't really understand what we need / want it for. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1 00/10] Flatpak + CI
Hi On Mon, Feb 11, 2019 at 2:21 PM Victor Toso wrote: > > Hi, > > On Mon, Feb 11, 2019 at 02:06:19PM +0100, Marc-André Lureau wrote: > > Hi > > > > On Mon, Feb 11, 2019 at 1:59 PM Victor Toso wrote: > > > > > > Hi, > > > > > > On Mon, Feb 11, 2019 at 01:09:41PM +0100, Marc-André Lureau wrote: > > > > > Having a virt-viewer flatpak does not mean _not_ having a > > > > > spicy.flatkpak (to me); one is full featured spice client while > > > > > the other a testing tool... > > > > > > > > It looks like a lot of duplication of flatpak effort. Maybe you could > > > > simply ship spicy in virt-viewer flatpak, so it could be run from > > > > command line (please no .desktop) > > > > > > Is the fact that we are installing a .desktop for spicy the only > > > issue here or you don't want to see a flatpak of spicy in the > > > gitlab-ci anyway? (btw, I'm not planning to upload this to any > > > flatpak provider). > > > > What's the point in building a spice-gtk flatpak then, when you > > have virt-viewer flatpak? > > You replied my question with another question. > > My main motivation is that spicy is self contained in spice-gtk, > smaller and targeted to be a testing tool, so, testing spice-gtk > changes. I see, you would like CI build version readily available. (kind of a waste of space to me, but it may be useful) virt-viewer with only --enable-spice-gtk shouldn't have much more dependencies though. > If I add virt-viewer -> flatpak or msi installer to gitlab-ci's > artifacts, that's out of scope of spice-gtk although I'll be > using it all the time... Oh you are thinking about building virt-viewer from spice-gtk CI? interesting... I wonder if there are mechanisms already to trigger rebuilds of dependent projects, I am pretty sure there are solutions to that. And flatpak build can pull from upstream repository master I guess. -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1 00/10] Flatpak + CI
Hi On Mon, Feb 11, 2019 at 1:59 PM Victor Toso wrote: > > Hi, > > On Mon, Feb 11, 2019 at 01:09:41PM +0100, Marc-André Lureau wrote: > > > Having a virt-viewer flatpak does not mean _not_ having a > > > spicy.flatkpak (to me); one is full featured spice client while > > > the other a testing tool... > > > > It looks like a lot of duplication of flatpak effort. Maybe you could > > simply ship spicy in virt-viewer flatpak, so it could be run from > > command line (please no .desktop) > > Is the fact that we are installing a .desktop for spicy the only > issue here or you don't want to see a flatpak of spicy in the > gitlab-ci anyway? (btw, I'm not planning to upload this to any > flatpak provider). What's the point in building a spice-gtk flatpak then, when you have virt-viewer flatpak? -- Marc-André Lureau ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
