Re: [Spice-devel] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list

2019-03-25 Thread Yuri Benditovich 
Yes, it looks like this finally does the same thing.
From my personal point of view, this is less obvious than previous
code, so I would add 2 comments:
that we unreference the device from the new list
that we add reference to the device from the old list, it will be
dereferenced on next line when the old list freed


On Mon, Mar 25, 2019 at 6:17 PM Christophe Fergeau  wrote:
>
> When getting a new list of USB devices after a WM_DEVICECHANGE event,
> libusb_device which were already present before the event may be
> represented by a different instance than the one we got in the new list.
>
> Since other layers of spice-gtk may be using that older instance of
> libusb_device, this commit makes sure that we always keep the older
> instance in GUdevClient::udev_list.
>
> At the moment, this should not be making any difference, but will make
> things more consistent later on.
>
> Based on a patch from Yuri Benditovich.
>
> Signed-off-by: Christophe Fergeau 
> ---
>
> Code is a bit longer this way, but imo it makes more sense to modify the
> list when it's being updated, rather than modifying it during
> notification.
>
>
>  src/win-usb-dev.c | 22 --
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 192ab17f..a16bd3ae 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -423,6 +423,25 @@ static gint compare_libusb_devices(gconstpointer a, 
> gconstpointer b)
>  return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
>  }
>
> +static void update_device_list(GUdevClient *self, GList *new_device_list)
> +{
> +GList *dev;
> +
> +for (dev = g_list_first(new_device_list); dev != NULL; dev = 
> g_list_next(dev)) {
> +GList *found = g_list_find_custom(self->priv->udev_list, dev->data, 
> compare_libusb_devices);
> +if (found != NULL) {
> +/* keep old existing libusb_device in the new list, when
> +   usb-dev-manager will maintain its own list of libusb_device,
> +   these lists will be completely consistent */
> +libusb_unref_device(dev->data);
> +dev->data = libusb_ref_device(found->data);
> +}
> +}
> +
> +g_udev_client_free_device_list(>priv->udev_list);
> +self->priv->udev_list = new_device_list;
> +}
> +
>  static void notify_dev_state_change(GUdevClient *self,
>  GList *old_list,
>  GList *new_list,
> @@ -469,8 +488,7 @@ static void handle_dev_change(GUdevClient *self)
>  notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
>
>  /* keep most recent info: free previous list, and keep current list */
> -g_udev_client_free_device_list(>udev_list);
> -priv->udev_list = now_devs;
> +update_device_list(self, now_devs);
>  }
>
>  static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, 
> LPARAM lparam)
> --
> 2.21.0
>
> ___
> 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] [spice-gtk v2 10/13] win-usb-dev: maintain list of libusb devices

2019-03-25 Thread Christophe Fergeau 
On Thu, Mar 21, 2019 at 05:16:03PM +0200, Yuri Benditovich wrote:
> > > @@ -412,10 +428,17 @@ static void notify_dev_state_change(GUdevClient 
> > > *self,
> > >  GList *dev;
> > >
> > >  for (dev = g_list_first(old_list); dev != NULL; dev = 
> > > g_list_next(dev)) {
> > > -if (g_list_find_custom(new_list, dev->data, 
> > > gudev_devices_differ) == NULL) {
> > > -/* Found a device that changed its state */
> > > +GList *found = g_list_find_custom(new_list, dev->data, 
> > > compare_libusb_devices);
> > > +if (found == NULL) {
> > >  g_udev_device_print(dev->data, add ? "add" : "remove");
> > > -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, 
> > > add);
> > > +g_udev_notify_device(self, dev->data, add);
> > > +} else if (add) {
> > > +/* keep old existing libusb_device in the new list, when
> > > +   usb-dev-manager will maintain its own list of 
> > > libusb_device,
> > > +   these lists will be completely consistent */
> > > +libusb_device *temp = found->data;
> > > +found->data = dev->data;
> > > +dev->data = temp;
> >
> > I'm still annoyed by this slightly complicated code in a method named 
> > notify_dev_state_change
> > (more on this below)
> >
> > >  }
> > >  }
> > >  }
> > > @@ -446,7 +469,8 @@ static void handle_dev_change(GUdevClient *self)
> > >  /* Unregister devices that are not present anymore */
> > >  notify_dev_state_change(self, priv->udev_list, now_devs, FALSE);
> > >
> > > -/* Register newly inserted devices */
> > > +/* report newly inserted devices and swap pointers to existing 
> > > devices:
> > > +   keep old pointers in now_devs list, keep new pointers in 
> > > udev_list */
> > >  notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
> > >
> > >  /* keep most recent info: free previous list, and keep current list 
> > > */
> > >  g_udev_client_free_device_list(>udev_list);
> > >  priv->udev_list = now_devs;
> >
> > Maybe these 2 lines + the code to replace new libusb_device pointers
> > with the old ones could be moved to a new
> > g_udev_client_update_device_list(self, now_devs);
> > helper?
> >
> 
> From my point of view, this will make the code more complicated,
> as additional procedure should traverse both lists again.

I made an attempt at that, see the 2 patches in answer to this thread.
I did not test them though :-/ Let me know what you think.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list

2019-03-25 Thread Christophe Fergeau 
When getting a new list of USB devices after a WM_DEVICECHANGE event,
libusb_device which were already present before the event may be
represented by a different instance than the one we got in the new list.

Since other layers of spice-gtk may be using that older instance of
libusb_device, this commit makes sure that we always keep the older
instance in GUdevClient::udev_list.

At the moment, this should not be making any difference, but will make
things more consistent later on.

Based on a patch from Yuri Benditovich.

Signed-off-by: Christophe Fergeau 
---

Code is a bit longer this way, but imo it makes more sense to modify the
list when it's being updated, rather than modifying it during
notification.


 src/win-usb-dev.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
index 192ab17f..a16bd3ae 100644
--- a/src/win-usb-dev.c
+++ b/src/win-usb-dev.c
@@ -423,6 +423,25 @@ static gint compare_libusb_devices(gconstpointer a, 
gconstpointer b)
 return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
 }
 
+static void update_device_list(GUdevClient *self, GList *new_device_list)
+{
+GList *dev;
+
+for (dev = g_list_first(new_device_list); dev != NULL; dev = 
g_list_next(dev)) {
+GList *found = g_list_find_custom(self->priv->udev_list, dev->data, 
compare_libusb_devices);
+if (found != NULL) {
+/* keep old existing libusb_device in the new list, when
+   usb-dev-manager will maintain its own list of libusb_device,
+   these lists will be completely consistent */
+libusb_unref_device(dev->data);
+dev->data = libusb_ref_device(found->data);
+}
+}
+
+g_udev_client_free_device_list(>priv->udev_list);
+self->priv->udev_list = new_device_list;
+}
+
 static void notify_dev_state_change(GUdevClient *self,
 GList *old_list,
 GList *new_list,
@@ -469,8 +488,7 @@ static void handle_dev_change(GUdevClient *self)
 notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
 
 /* keep most recent info: free previous list, and keep current list */
-g_udev_client_free_device_list(>udev_list);
-priv->udev_list = now_devs;
+update_device_list(self, now_devs);
 }
 
 static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, 
LPARAM lparam)
-- 
2.21.0

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

[Spice-devel] [spice-gtk 1/2] win-usb-dev: maintain list of libusb devices

2019-03-25 Thread Christophe Fergeau 
From: Yuri Benditovich 

Change internal device list to maintain libusb devices
instead of GUdevDevice objects. Create temporary
GUdevDevice object only for indication to usb-dev-manager.

Signed-off-by: Yuri Benditovich 
Acked-by: Christophe Fergeau 
---
Changes since version on the list:
- notify_dev_state_change no longer modify the device list

 src/win-usb-dev.c | 73 +--
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
index 7f0213d4..192ab17f 100644
--- a/src/win-usb-dev.c
+++ b/src/win-usb-dev.c
@@ -95,7 +95,7 @@ static void g_udev_device_print_list(GList *l, const gchar 
*msg);
 #else
 static void g_udev_device_print_list(GList *l, const gchar *msg) {}
 #endif
-static void g_udev_device_print(GUdevDevice *udev, const gchar *msg);
+static void g_udev_device_print(libusb_device *dev, const gchar *msg);
 
 static gboolean g_udev_skip_search(libusb_device *dev);
 
@@ -129,8 +129,6 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
 gssize rc;
 libusb_device **lusb_list, **dev;
 GUdevClientPrivate *priv;
-GUdevDeviceInfo *udevinfo;
-GUdevDevice *udevice;
 ssize_t n;
 
 g_return_val_if_fail(G_UDEV_IS_CLIENT(self), -1);
@@ -155,10 +153,7 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
 if (g_udev_skip_search(*dev)) {
 continue;
 }
-udevinfo = g_new0(GUdevDeviceInfo, 1);
-get_usb_dev_info(*dev, udevinfo);
-udevice = g_udev_device_new(udevinfo);
-*devs = g_list_prepend(*devs, udevice);
+*devs = g_list_prepend(*devs, libusb_ref_device(*dev));
 n++;
 }
 libusb_free_device_list(lusb_list, 1);
@@ -166,11 +161,20 @@ g_udev_client_list_devices(GUdevClient *self, GList 
**devs,
 return n;
 }
 
+static void unreference_libusb_device(gpointer data)
+{
+libusb_unref_device((libusb_device *)data);
+}
+
 static void g_udev_client_free_device_list(GList **devs)
 {
 g_return_if_fail(devs != NULL);
 if (*devs) {
-g_list_free_full(*devs, g_object_unref);
+/* the unreference_libusb_device method is required as
+ * libusb_unref_device calling convention differs from glib's
+ * see 558c967ec
+ */
+g_list_free_full(*devs, unreference_libusb_device);
 *devs = NULL;
 }
 }
@@ -246,9 +250,22 @@ static void 
g_udev_client_initable_iface_init(GInitableIface *iface)
 iface->init = g_udev_client_initable_init;
 }
 
+static void g_udev_notify_device(GUdevClient *self, libusb_device *dev, 
gboolean add)
+{
+GUdevDeviceInfo *udevinfo;
+GUdevDevice *udevice;
+udevinfo = g_new0(GUdevDeviceInfo, 1);
+if (get_usb_dev_info(dev, udevinfo)) {
+udevice = g_udev_device_new(udevinfo);
+g_signal_emit(self, signals[UEVENT_SIGNAL], 0, udevice, add);
+} else {
+g_free(udevinfo);
+}
+}
+
 static void report_one_device(gpointer data, gpointer self)
 {
-g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);
+g_udev_notify_device(self, data, TRUE);
 }
 
 void g_udev_client_report_devices(GUdevClient *self)
@@ -388,18 +405,20 @@ static gboolean get_usb_dev_info(libusb_device *dev, 
GUdevDeviceInfo *udevinfo)
 }
 
 /* comparing bus:addr and vid:pid */
-static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
+static gint compare_libusb_devices(gconstpointer a, gconstpointer b)
 {
-GUdevDeviceInfo *ai, *bi;
+libusb_device *a_dev = (libusb_device *)a;
+libusb_device *b_dev = (libusb_device *)b;
+struct libusb_device_descriptor a_desc, b_desc;
 gboolean same_bus, same_addr, same_vid, same_pid;
 
-ai = G_UDEV_DEVICE(a)->priv->udevinfo;
-bi = G_UDEV_DEVICE(b)->priv->udevinfo;
+libusb_get_device_descriptor(a_dev, _desc);
+libusb_get_device_descriptor(b_dev, _desc);
 
-same_bus = (ai->bus == bi->bus);
-same_addr = (ai->addr == bi->addr);
-same_vid = (ai->vid == bi->vid);
-same_pid = (ai->pid == bi->pid);
+same_bus = (libusb_get_bus_number(a_dev) == libusb_get_bus_number(b_dev));
+same_addr = (libusb_get_device_address(a_dev) == 
libusb_get_device_address(b_dev));
+same_vid = (a_desc.idVendor == b_desc.idVendor);
+same_pid = (a_desc.idProduct == b_desc.idProduct);
 
 return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
 }
@@ -412,10 +431,10 @@ static void notify_dev_state_change(GUdevClient *self,
 GList *dev;
 
 for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
-if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == 
NULL) {
-/* Found a device that changed its state */
+GList *found = g_list_find_custom(new_list, dev->data, 
compare_libusb_devices);
+if (found == NULL) {
 g_udev_device_print(dev->data, add ? "add" : "remove");
-g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, add);
+

Re: [Spice-devel] [PATCH spice-space] Update support.rst

2019-03-25 Thread Uri Lublin 

Hi Snir,

Looks good to me.

Some suggestions at the bottom (inlined).

Feel free to fix/ignore and push with Frediano's ack.

On 3/24/19 6:04 PM, Snir Sheriber wrote:

---

Due to outdated links reported Daniel P. Berrangé in
"[Spice-devel] Bug tracker links outdated"

In addition i did changes in the text so i sent it for review


---
  support.rst | 20 
  1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/support.rst b/support.rst
index 728c9d1..0e5c4f5 100644
--- a/support.rst
+++ b/support.rst
@@ -7,12 +7,8 @@ Support
  .. _Spice User Manual: spice-user-manual.html
  .. _spice for newbies: {filename}/static/docs/spice_for_newbies.pdf
  .. _Spice list archive: http://lists.freedesktop.org/archives/spice-devel/
-.. _opened bugs: 
https://bugs.freedesktop.org/describecomponents.cgi?product=Spice
-.. _bug-tracker: https://bugs.freedesktop.org/enter_bug.cgi?product=Spice
-.. _feature-tracker: 
https://bugs.freedesktop.org/enter_bug.cgi?product=Spice=RFE
+.. _issues: https://gitlab.freedesktop.org/groups/spice/-/issues
  .. _Features: features.html
-.. _search: https://bugs.freedesktop.org/query.cgi?product=Spice
-.. _opened RFEs: 
https://bugs.freedesktop.org/buglist.cgi?component=RFE_id=560478=Spice=---
  
  Using Spice for the first time?

  +++
@@ -30,20 +26,20 @@ Spice project would like to have your feedback - either 
negative or positive. Pl
  
  Found a bug? Want to help and report?

  +
-#. Look at `opened bugs`_ to see whether it is a known bug.
+#. Look at `issues`_ to see whether it is a known bug.
  
-   - There is no need to open a new bug in case it is a known bug, but optionally you can add your comment to the existing bug report or add yourself to the CC list.

+   - There is no need to open a new issue in case it is already known, but 
optionally you can add your comment to the existing issue.
 - In case you are not using the latest version of Spice, upgrade and see 
if it was resolved.
  
-#. Open a new bug report in Spice `bug-tracker`_ that follow these guidelines:

+#. Create a new issue under the relevant project in Spice `issues`_ and follow 
these guidelines:
  
 - Write clear and detailed description of the bug.

 - Provide instructions on how to reproduce the bug.
-   - Specify the relevant components and their versions.
+   - Specify all relevant components and their versions.
 - Attach relevant logs.
  
- - Spice server send its log messages to stdout, so you will need to redirect QEMU stdout to a file in order to have a server log file.

- - Linux Spice client log is /tmp/spicec.log, Windows Spice client log is 
in %temp%\spicec.log.
+ - Spice server send its log messages to stdout, use G_MESSAGES_DEBUG=all 
and redirect QEMU stdout to a file in order to have a server log file.


s/send/sends/

Maybe replace with: use G_MESSAGES_DEBUG=all for debug-level logging,
possibly moving the "redirect ..." to a separate line.
Also maybe redirect also stderr to that file.


+ - To get Spice client debug messages to stdout run it with --spice-debug.


I would make the client similar to the server:
- Spice client sends its log messages to stdout, use --spice-debug to 
enable debug level logging.

(or also add the "redirect stdout to a file" ... if you want)

Uri.

  
 - Provide the necessary information:
  
@@ -55,4 +51,4 @@ Your help for tracking down the bug is very important, so please try to be as co
  
  Like to have a new feature?

  +++
-First look at Spice `opened RFEs`_ or `search`_ to see whether it was already 
requested. In addition, refer to the Features_ page. If it was not requested 
you can submit your request in Spice `feature-tracker`_ using a clear and 
detailed description.
+First search `issues`_ to see whether it was already requested. In addition, 
refer to the Features_ page. If it was not requested you can submit a new issue 
using a clear and detailed description.

___
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

2019-03-25 Thread Jakub Janku 
Hi,

On Mon, Mar 25, 2019, 11:34 AM Marc-André Lureau 
wrote:

> 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.
>

I think it's necessary as long as you want to use the new GObject macro.

G_DECLARE_FINAL_TYPE typedefs VdagentClipboards as struct _VDAgentClipboards
afaik.

Cheers,
Jakub

>
> >
> > > +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..cd8eacb 100644
> > > --- a/src/vdagent/clipboard.h
> > > +++ b/src/vdagent/clipboard.h
> > > @@ -19,16 +19,18 @@
> > >  #ifndef __VDAGENT_CLIPBOARD_H
> > >  #define __VDAGENT_CLIPBOARD_H
> > >
> > > -#include 
> > > +#include 
> > >
> > >  #include "x11.h"
> > >  #include "udscs.h"
> > >
> > > -typedef struct

Re: [Spice-devel] [PATCH linux/vd-agent 08/11] clipboard: gobject-ify VDAgentClipboards

2019-03-25 Thread Marc-André Lureau 
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..cd8eacb 100644
> > --- a/src/vdagent/clipboard.h
> > +++ b/src/vdagent/clipboard.h
> > @@ -19,16 +19,18 @@
> >  #ifndef __VDAGENT_CLIPBOARD_H
> >  #define __VDAGENT_CLIPBOARD_H
> >
> > -#include 
> > +#include 
> >
> >  #include "x11.h"
> >  #include "udscs.h"
> >
> > -typedef struct VDAgentClipboards VDAgentClipboards;
> > +#define VDAGENT_TYPE_CLIPBOARDS vdagent_clipboards_get_type()
> > +G_DECLARE_FINAL_TYPE(VDAgentClipboards, vdagent_clipboards, VDAGENT,
> > CLIPBOARDS, GObject)
> >
> > -VDAgentClipboards *vdagent_clipboards_init(struct vdagent_x11  *x11,
> > -   struct udscs_connection *conn);
> > -void vdagent_clipboards_finalize(VDAgentClipboards *c, gboolean 
> > conn_alive);
> > +VDAgentClipboards *vdagent_clipboards_new(struct vdagent_x11 *x11);
> > +
> > +void

Re: [Spice-devel] [PATCH spice-space] Update support.rst

2019-03-25 Thread Frediano Ziglio 

> ---
> 
> Due to outdated links reported Daniel P. Berrangé in
> "[Spice-devel] Bug tracker links outdated"
> 

Maybe adding a "Reported-by" in the commit log?

> In addition i did changes in the text so i sent it for review
> 
> 
> ---
>  support.rst | 20 
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/support.rst b/support.rst
> index 728c9d1..0e5c4f5 100644
> --- a/support.rst
> +++ b/support.rst
> @@ -7,12 +7,8 @@ Support
>  .. _Spice User Manual: spice-user-manual.html
>  .. _spice for newbies: {filename}/static/docs/spice_for_newbies.pdf
>  .. _Spice list archive: http://lists.freedesktop.org/archives/spice-devel/
> -.. _opened bugs:
> https://bugs.freedesktop.org/describecomponents.cgi?product=Spice
> -.. _bug-tracker: https://bugs.freedesktop.org/enter_bug.cgi?product=Spice
> -.. _feature-tracker:
> https://bugs.freedesktop.org/enter_bug.cgi?product=Spice=RFE
> +.. _issues: https://gitlab.freedesktop.org/groups/spice/-/issues
>  .. _Features: features.html
> -.. _search: https://bugs.freedesktop.org/query.cgi?product=Spice
> -.. _opened RFEs:
> https://bugs.freedesktop.org/buglist.cgi?component=RFE_id=560478=Spice=---
>  
>  Using Spice for the first time?
>  +++
> @@ -30,20 +26,20 @@ Spice project would like to have your feedback - either
> negative or positive. Pl
>  
>  Found a bug? Want to help and report?
>  +
> -#. Look at `opened bugs`_ to see whether it is a known bug.
> +#. Look at `issues`_ to see whether it is a known bug.
>  
> -   - There is no need to open a new bug in case it is a known bug, but
> optionally you can add your comment to the existing bug report or add
> yourself to the CC list.
> +   - There is no need to open a new issue in case it is already known, but
> optionally you can add your comment to the existing issue.
> - In case you are not using the latest version of Spice, upgrade and see
> if it was resolved.
>  
> -#. Open a new bug report in Spice `bug-tracker`_ that follow these
> guidelines:
> +#. Create a new issue under the relevant project in Spice `issues`_ and
> follow these guidelines:
>  
> - Write clear and detailed description of the bug.
> - Provide instructions on how to reproduce the bug.
> -   - Specify the relevant components and their versions.
> +   - Specify all relevant components and their versions.
> - Attach relevant logs.
>  
> - - Spice server send its log messages to stdout, so you will need to
> redirect QEMU stdout to a file in order to have a server log file.
> - - Linux Spice client log is /tmp/spicec.log, Windows Spice client log
> is in %temp%\spicec.log.
> + - Spice server send its log messages to stdout, use
> G_MESSAGES_DEBUG=all and redirect QEMU stdout to a file in order to have a
> server log file.
> + - To get Spice client debug messages to stdout run it with
> --spice-debug.
>  
> - Provide the necessary information:
>  
> @@ -55,4 +51,4 @@ Your help for tracking down the bug is very important, so
> please try to be as co
>  
>  Like to have a new feature?
>  +++
> -First look at Spice `opened RFEs`_ or `search`_ to see whether it was
> already requested. In addition, refer to the Features_ page. If it was not
> requested you can submit your request in Spice `feature-tracker`_ using a
> clear and detailed description.
> +First search `issues`_ to see whether it was already requested. In addition,
> refer to the Features_ page. If it was not requested you can submit a new
> issue using a clear and detailed description.

Patch looks good for me, some minor doubts about splitting and grammar,
beside that

Acked-by: Frediano Ziglio 

Frediano
___
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

2019-03-25 Thread Frediano Ziglio 
> 
> 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?

> +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

> +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..cd8eacb 100644
> --- a/src/vdagent/clipboard.h
> +++ b/src/vdagent/clipboard.h
> @@ -19,16 +19,18 @@
>  #ifndef __VDAGENT_CLIPBOARD_H
>  #define __VDAGENT_CLIPBOARD_H
>  
> -#include 
> +#include 
>  
>  #include "x11.h"
>  #include "udscs.h"
>  
> -typedef struct VDAgentClipboards VDAgentClipboards;
> +#define VDAGENT_TYPE_CLIPBOARDS vdagent_clipboards_get_type()
> +G_DECLARE_FINAL_TYPE(VDAgentClipboards, vdagent_clipboards, VDAGENT,
> CLIPBOARDS, GObject)
>  
> -VDAgentClipboards *vdagent_clipboards_init(struct vdagent_x11  *x11,
> -   struct udscs_connection *conn);
> -void vdagent_clipboards_finalize(VDAgentClipboards *c, gboolean conn_alive);
> +VDAgentClipboards *vdagent_clipboards_new(struct vdagent_x11 *x11);
> +
> +void vdagent_clipboards_set_conn(VDAgentClipboards *self,
> + struct udscs_connection *conn);
>  
>  void vdagent_clipboard_request(VDAgentClipboards *c, guint sel_id, guint
>  type);
>  
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index 61aeac7..bfc0d5f 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -165,8 +165,8 @@ static void vdagent_quit_loop(VDAgent *agent)
>  {
>  /* other GMainLoop(s) might be running, quit them before agent->loop */
>  if