[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/5] block/nbd.c: Add yank feature
From: |
Lukas Straub |
Subject: |
Re: [PATCH 3/5] block/nbd.c: Add yank feature |
Date: |
Fri, 15 May 2020 11:48:18 +0200 |
On Tue, 12 May 2020 09:54:58 +0100
Daniel P. Berrangé <address@hidden> wrote:
> On Mon, May 11, 2020 at 07:05:24PM +0200, Lukas Straub wrote:
> > On Mon, 11 May 2020 17:19:09 +0100
> > "Dr. David Alan Gilbert" <address@hidden> wrote:
> >
> > > * Lukas Straub (address@hidden) wrote:
> > > > Add yank option, pass it to the socket-channel and register a yank
> > > > function which sets s->state = NBD_CLIENT_QUIT. This is the same
> > > > behaviour as if an error occured.
> > > >
> > > > Signed-off-by: Lukas Straub <address@hidden>
> > >
> > > > +static void nbd_yank(void *opaque)
> > > > +{
> > > > + BlockDriverState *bs = opaque;
> > > > + BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> > > > +
> > > > + atomic_set(&s->state, NBD_CLIENT_QUIT);
> > >
> > > I think I was expecting a shutdown on the socket here - why doesn't it
> > > have one?
> >
> > For nbd, we register two yank functions: This one and we enable
> > the yank feature on the qio channel (see function
> > nbd_establish_connection below).
>
> As mentioned on the earlier patch, I don't want to see any yank
> code in the QIOChannel object directly. This nbd_yank function
> can simply call the qio_channel_shutdown() function directly
> and avoid need for modifying the QIOChannel object with yank
> support.
Hi,
Looking at it again, the problem is not with registering the yank functions,
but with tracking the lifetime of it. Suppose we add qio_channel_shutdown to
the yank_nbd function. Then we need to unregister it whenever the QIOChannel
object is freed.
In the code that would lead to the following constructs in a lot of places:
if (local_err) {
yank_unregister_function(s->yank_name, yank_nbd, bs);
object_unref(OBJECT(sioc));
error_propagate(errp, local_err);
return NULL;
}
If you don't want the code in QIOChannel I guess I can create a subclass
(YankableChannelSocket?) of QIOChannelSocket. What do you think?
Regards,
Lukas Straub
> This will make the NBD code clearer too, as we can see exactly
> what actions are performed at NBD layer when a yank happens,
> which is what David was not seeing clearly here.
>
> Regards,
> Daniel
pgpbb62dzHvGS.pgp
Description: OpenPGP digital signature
- [PATCH 1/5] Introduce yank feature, (continued)
- [PATCH 3/5] block/nbd.c: Add yank feature, Lukas Straub, 2020/05/11
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Dr. David Alan Gilbert, 2020/05/11
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Lukas Straub, 2020/05/11
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Dr. David Alan Gilbert, 2020/05/11
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Daniel P . Berrangé, 2020/05/12
- Re: [PATCH 3/5] block/nbd.c: Add yank feature,
Lukas Straub <=
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Daniel P . Berrangé, 2020/05/15
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Lukas Straub, 2020/05/15
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Daniel P . Berrangé, 2020/05/15
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Lukas Straub, 2020/05/15
- Re: [PATCH 3/5] block/nbd.c: Add yank feature, Daniel P . Berrangé, 2020/05/15
[PATCH 4/5] chardev/char-socket.c: Add yank feature, Lukas Straub, 2020/05/11
[PATCH 5/5] migration: Add yank feature, Lukas Straub, 2020/05/11
Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu, Daniel P . Berrangé, 2020/05/11