Re: [Qemu-devel] [PATCH] Introduce NBD named exports.

[Kevin Wolf <kwolf@redhat.com>]

Am 25.08.2010 00:21, schrieb Laurent Vivier:
> Le mardi 24 août 2010 à 10:59 +0200, Kevin Wolf a écrit :
>> Am 24.08.2010 03:04, schrieb Laurent Vivier:
>>> This patch allows to connect Qemu using NBD protocol to an nbd-server
>>> using named exports.
>>>
>>> For instance, if on the host "isoserver", in /etc/nbd-server/config, you have:
>>>
>>> [generic]
>>> [debian-500-ppc-netinst]
>>>         exportname = /ISO/debian-500-powerpc-netinst.iso
>>> [Fedora-10-ppc-netinst]
>>>         exportname = /ISO/Fedora-10-ppc-netinst.iso
>>>
>>> You can connect to it, using:
>>>
>>>     qemu -cdrom nbd:isoserver:exportname=debian-500-ppc-netinst
>>>     qemu -cdrom nbd:isoserver:exportname=Fedora-10-ppc-netinst
>>>
>>> NOTE: you need at least nbd-server 2.9.18
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>  block/nbd.c   |   57 +++++++++++++++++++--------
>>>  nbd.c         |  119 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>  nbd.h         |    5 ++-
>>>  qemu-doc.texi |    7 +++
>>>  qemu-nbd.c    |    2 +-
>>>  5 files changed, 158 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/block/nbd.c b/block/nbd.c
>>> index a1ec123..ee22b47 100644
>>> --- a/block/nbd.c
>>> +++ b/block/nbd.c
>>> @@ -42,55 +42,78 @@ typedef struct BDRVNBDState {
>>>  static int nbd_open(BlockDriverState *bs, const char* filename, int flags)
>>>  {
>>>      BDRVNBDState *s = bs->opaque;
>>> +
>>> +    #define EN_OPTSTR ":exportname="
>>
>> Hm, this makes EN_OPTSTR look like a local variable, but it's actually a
>> macro which is valid after the end of the function. So maybe move it out?
> 
> I agree, but have a look at curl_open().

Seems I should have reviewed the curl patch when it came in. :-)

>> Maybe using strlen(EN_OPTSTR) would be clearer? At first I missed the
>> fact that sizeof includes the null byte, but maybe it's just me.
> 
> I think sizeof() is resolved at compile time whereas strlen() it is at
> runtime. For me, it is better, it like to have a constant.

gcc is smart enough to make strlen of a literal string a constant, too.
Besides, opening a connection isn't really performance critical.

I guess it's a matter of taste, though. So if you like it better as it
is, just leave it.

>> I don't have a new enough ndbserver to test if it actually works (and I
> 
> You can test against an old nbdserver to check it is always working ;-)
> (or qemu-nbd).

Sure, I can test for regressions in the old thing.

>> also haven't checked the spec for the new parts), so I'll have to trust
>> your tests there.
> 
> There is no spec (what is a spec ;-) ?), only a reference implementation
> in nbd-client.c and nbd-server.c from http://nbd.sf.net.

Oh right, I forgot, the world outside qemu is just as bad. :-)

Kevin