QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: bharatlkmlkvm@gmail.com, qemu-devel@nongnu.org,
	Coiby Xu <coiby.xu@gmail.com>
Subject: Re: [PATCH v2 1/5] vhost-user block device backend
Date: Thu, 16 Jan 2020 15:20:33 +0100
Message-ID: <20200116142033.GF9470@linux.fritz.box> (raw)
In-Reply-To: <20200116135122.GI163546@stefanha-x1.localdomain>

[-- Attachment #1: Type: text/plain, Size: 2688 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14 14:06 [PATCH v2 0/5] vhost-user block device backend implementation Coiby Xu
2020-01-14 14:06 ` [PATCH v2 1/5] vhost-user block device backend Coiby Xu
2020-01-16 13:51   ` Stefan Hajnoczi
2020-01-16 14:20     ` Kevin Wolf [this message]
2020-02-14  3:07     ` Coiby Xu
2020-01-16 13:56   ` Kevin Wolf
2020-01-14 14:06 ` [PATCH v2 2/5] extend libvhost to support IOThread Coiby Xu
2020-01-14 14:06 ` [PATCH v2 3/5] a standone-alone tool to directly share disk image file via vhost-user protocol Coiby Xu
2020-01-16 14:04   ` Stefan Hajnoczi
2020-01-17  8:12     ` Coiby Xu
2020-01-17 10:11       ` Kevin Wolf
2020-01-31 16:42         ` Coiby Xu
2020-02-02  9:33           ` Kevin Wolf
2020-02-13  1:00             ` Coiby Xu
2020-01-14 14:06 ` [PATCH v2 4/5] new qTest case for the vhost-user-blk device backend Coiby Xu
2020-01-14 14:06 ` [PATCH v2 5/5] building configuration files changes Coiby Xu
2020-01-16 11:07   ` Kevin Wolf

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200116142033.GF9470@linux.fritz.box \
    --to=kwolf@redhat.com \
    --cc=bharatlkmlkvm@gmail.com \
    --cc=coiby.xu@gmail.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git