[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/5] vhost-user block device backend
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v2 1/5] vhost-user block device backend |
Date: |
Thu, 16 Jan 2020 15:20:33 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
Am 16.01.2020 um 14:51 hat Stefan Hajnoczi geschrieben:
> > +static void vu_set_unix_socket(Object *obj, const char *value,
> > + Error **errp)
> > +{
> > + VubDev *vus = VHOST_USER_SERVER(obj);;
> > +
> > + if (vus->unix_socket) {
> > + error_setg(errp, "unix_socket property already set");
> > + return;
> > + }
> > +
> > + vus->unix_socket = g_strdup(value);
> > + vhost_user_server_start(vus, value, vus->name,
> > + vus->writable, errp);
>
> Property setters should only perform input validation and store the
> data. Actions like creating network connections, opening files, etc
> should happen later in a UserCreatableClass->complete() callback.
>
> This is necessary because vus->writable is also a property and may be
> set after unix_socket. The ->complete() callback is called after all
> setters so it can access the final values of all properties.
>
> See iothread_class_init() and iothread_complete() for an example.
Ah, right, this is the correct way. I forgot about the existence of
.complete(), so please ignore what I wrote.
> > diff --git a/vl.c b/vl.c
> > index 86474a55c9..72ac506342 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2553,6 +2553,10 @@ static bool object_create_initial(const char *type,
> > QemuOpts *opts)
> > }
> > #endif
> >
> > + /* Reason: vhost-user-server property "name" */
> > + if (g_str_equal(type, "vhost-user-server")) {
> > + return false;
> > + }
>
> I don't understand why the "name" property introduces a creation order
> dependency. It's just a string and has no dependency on other
> command-line objects. Can you explain why this change is necessary?
I was confused at first, too, but it's just a naming problem: "name"
is what points to the block device to be exported. It should really be
"node-name".
> > +struct VuClient {
> > + VuDev parent;
> > + int refcount;
> > + VubDev *blk;
> > + QIOChannelSocket *sioc; /* The underlying data channel */
> > + QIOChannel *ioc; /* The current I/O channel */
> > + QTAILQ_ENTRY(VuClient) next;
> > + bool closed;
> > +};
> > +VubDev *vub_dev_find(const char *name);
>
> All -object instances already have an id=ID property. There is no need
> to declare a separate "name" property. Please look at iothread_by_id()
> and iothread_get_id() for examples.
It's questionable to me if this function is needed at all. It is only
used in vhost_user_server_start() to make sure that you don't export a
node twice - but what's even the problem with exporting it twice?
Kevin
signature.asc
Description: PGP signature
[PATCH v2 4/5] new qTest case for the vhost-user-blk device backend, Coiby Xu, 2020/01/14
[PATCH v2 5/5] building configuration files changes, Coiby Xu, 2020/01/14