ui: add standalone VNC server over D-Bus

[PATCH 08/60] ui/vnc-jobs: narrow taking the lock when pushing empty jobs

Posted by Marc-André Lureau 2 weeks, 6 days ago

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc-jobs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 28a4738f1ec..ae925171fae 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -122,14 +122,14 @@ static void vnc_job_free(VncJob *job)
 
 void vnc_job_push(VncJob *job)
 {
-    vnc_lock_queue(queue);
     if (QLIST_EMPTY(&job->rectangles)) {
         vnc_job_free(job);
     } else {
+        vnc_lock_queue(queue);
         QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
         qemu_cond_broadcast(&queue->cond);
+        vnc_unlock_queue(queue);
     }
-    vnc_unlock_queue(queue);
 }
 
 static bool vnc_has_job_locked(VncState *vs)

-- 
2.53.0

Re: [PATCH 08/60] ui/vnc-jobs: narrow taking the lock when pushing empty jobs

Posted by Daniel P. Berrangé 1 week, 6 days ago

On Tue, Mar 17, 2026 at 12:50:22PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc-jobs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index 28a4738f1ec..ae925171fae 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -122,14 +122,14 @@ static void vnc_job_free(VncJob *job)
>  
>  void vnc_job_push(VncJob *job)
>  {
> -    vnc_lock_queue(queue);
>      if (QLIST_EMPTY(&job->rectangles)) {

All the APIs which write to job->rectangles do that in a block
that is protected by vnc_lock_queue, so narrowing this to allow
lock-less access of job->rectangles feels dubious, without some
justification in the commit message to explain why it is safe
for job->rectangles to be unprotected, while 'queue->jobs' still
needs protection.

>          vnc_job_free(job);
>      } else {
> +        vnc_lock_queue(queue);
>          QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
>          qemu_cond_broadcast(&queue->cond);
> +        vnc_unlock_queue(queue);
>      }
> -    vnc_unlock_queue(queue);
>  }
>  
>  static bool vnc_has_job_locked(VncState *vs)
> 
> -- 
> 2.53.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH 08/60] ui/vnc-jobs: narrow taking the lock when pushing empty jobs

Posted by Marc-André Lureau 1 week, 6 days ago

Hi

On Tue, Mar 24, 2026 at 5:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Mar 17, 2026 at 12:50:22PM +0400, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  ui/vnc-jobs.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > index 28a4738f1ec..ae925171fae 100644
> > --- a/ui/vnc-jobs.c
> > +++ b/ui/vnc-jobs.c
> > @@ -122,14 +122,14 @@ static void vnc_job_free(VncJob *job)
> >
> >  void vnc_job_push(VncJob *job)
> >  {
> > -    vnc_lock_queue(queue);
> >      if (QLIST_EMPTY(&job->rectangles)) {
>
> All the APIs which write to job->rectangles do that in a block
> that is protected by vnc_lock_queue, so narrowing this to allow
> lock-less access of job->rectangles feels dubious, without some
> justification in the commit message to explain why it is safe
> for job->rectangles to be unprotected, while 'queue->jobs' still
> needs protection.

The caller is vnc_update_client(). It creates the job and push damaged
rect. It shouldn't take the lock either. Only if it modified a queued
job it would need the lock, but it doesn't. I'll update the patch.

>
> >          vnc_job_free(job);
> >      } else {
> > +        vnc_lock_queue(queue);
> >          QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
> >          qemu_cond_broadcast(&queue->cond);
> > +        vnc_unlock_queue(queue);
> >      }
> > -    vnc_unlock_queue(queue);
> >  }
> >
> >  static bool vnc_has_job_locked(VncState *vs)
> >
> > --
> > 2.53.0
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
>
>


-- 
Marc-André Lureau