[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v2 13/22] monitor: separate QMP parser and dispatc
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [RFC v2 13/22] monitor: separate QMP parser and dispatcher |
Date: |
Thu, 19 Oct 2017 15:13:11 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Thu, Oct 19, 2017 at 02:36:49PM +0800, Peter Xu wrote:
> On Wed, Oct 18, 2017 at 05:31:15PM +0200, Stefan Hajnoczi wrote:
> > On Mon, Oct 16, 2017 at 03:50:39PM +0800, Peter Xu wrote:
> > > On Thu, Oct 12, 2017 at 01:50:45PM +0100, Stefan Hajnoczi wrote:
> > > > On Fri, Sep 29, 2017 at 11:38:35AM +0800, Peter Xu wrote:
> > > > > + qdict = qobject_to_qdict(req);
> > > > > + if (qdict) {
> > > > > + id = qdict_get(qdict, "id");
> > > > > + qobject_incref(id);
> > > > > + qdict_del(qdict, "id");
> > > > > + } /* else will fail qmp_dispatch() */
> > > > > +
> > > > > + req_obj = g_new0(QMPRequest, 1);
> > > > > + req_obj->mon = mon;
> > > > > + req_obj->id = id;
> > > > > + req_obj->req = req;
> > > > > +
> > > > > + /*
> > > > > + * Put the request to the end of queue so that requests will be
> > > > > + * handled in time order. Ownership for req_obj, req, id,
> > > > > + * etc. will be delivered to the handler side.
> > > > > + */
> > > > > + g_queue_push_tail(mon->qmp_requests, req_obj);
> > > > > +
> > > > > + /* Kick the dispatcher routine */
> > > > > + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > > >
> > > > How is thread-safety ensured when accessing qmp_requests?
> > >
> > > It's a GQueue. I assume GQueue is thread safe itself as long as
> > > g_thread_init() is called?
> >
> > No, glib data structures are not automatically thread-safe unless the
> > documentation says so.
> >
> > Here is the implementation where you can see that no locking is
> > performed:
> >
> > void
> > g_queue_push_tail (GQueue *queue,
> > gpointer data)
> > {
> > g_return_if_fail (queue != NULL);
> >
> > queue->tail = g_list_append (queue->tail, data);
> > if (queue->tail->next)
> > queue->tail = queue->tail->next;
> > else
> > queue->head = queue->tail;
> > queue->length++;
> > }
>
> Oops. Yes then I possibly need a lock... Thanks.
>
> I think maybe I can rename the Monitor.out_lock into a more general
> name, then to use it as a per-monitor lock (instead of introducing
> another one).
Up to you. I don't remember the details of out_lock usage well enough
to know whether using the lock for the queues is good or bad.
Stefan