Re: [Qemu-devel] [PATCH v6 6/7] block/nbd-client: nbd reconnect

[Vladimir Sementsov-Ogievskiy]

10.06.2019 15:38, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 6:17, Eric Blake wrote:
>>> +typedef struct NBDConnection {
>>> +    BlockDriverState *bs;
>>> +    SocketAddress *saddr;
>>> +    const char *export;
>>> +    QCryptoTLSCreds *tlscreds;
>>> +    const char *hostname;
>>> +    const char *x_dirty_bitmap;
>>> +} NBDConnection;
>> Can we put this type in a header, and use it instead of passing lots of
>> individual parameters to nbd_client_connect()?  Probably as a separate
>> pre-requisite cleanup patch.
>>
> 
> 
> Hmm, and then, include it into BDRVNBDState? I don't remember why didn't do
> it, and now I don't see any reason for it. We store this information anyway
> for the whole life of the driver..
> 
> So, if I'm going to refactor it, I have a question: is there a reason for
> layered BDRVNBDState:
> 
> typedef struct BDRVNBDState {
>      NBDClientSession client;
> 
>      /* For nbd_refresh_filename() */
>      SocketAddress *saddr;
>      char *export, *tlscredsid;
> } BDRVNBDState;
> 
> and use nbd_get_client_session everywhere instead of simple converting void 
> *opaque
> to State pointer like in qcow2 for example?
> 
> The only reason I can imagine is not to share the whole BDRVNBDState, and keep
> things we are using only in nbd.c to be available only in nbd.c.. But I've put
> saddr and export to NBDConnection to be used in nbd-client.c anyway. So, I 
> think
> it's a good moment to flatten and share BDRVNBDState and drop 
> nbd_get_client_session.
> 

And if we are here, what is exact purpose of splitting  nbd-client.c from nbd.c?

I see, it was long ago:

commit 2302c1cafb13df23938b098d9c6595de52ec2577
Author: Marc-André Lureau <address@hidden>
Date:   Sun Dec 1 22:23:41 2013 +0100

     Split nbd block client code


But still, it doesn't explain..

And all this leads to noop wrappers like this

static int nbd_co_flush(BlockDriverState *bs)
{
     return nbd_client_co_flush(bs);
}

static void nbd_detach_aio_context(BlockDriverState *bs)
{
     nbd_client_detach_aio_context(bs);
}

static void nbd_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
{
     nbd_client_attach_aio_context(bs, new_context);
}


or need of defining driver callbacks in header file, instead of keeping them 
together
with driver struct definition and static (like other block drivers)...


And names of these two files always confused me.. nbd.c is nbd client block 
driver, and
driver is defined here.. But we have nbd-client.c which defines nbd client 
driver too.
And we also have nbd/client.c (OK, this split I understand of course, but it 
increases
confusion anyway).

-- 
Best regards,
Vladimir