Re: [Nbd] Current HEAD fails make check

[Wouter Verhelst <w@...112...>]

Hi,

On Tue, May 31, 2011 at 09:14:08PM +0100, Alex Bligh wrote:
> Wouter,
> 
> --On 31 May 2011 08:41:43 +0100 Alex Bligh <alex@...872...> wrote:
> 
> > I think it's the change in the port option.
>
> I've done some debugging on this. Essentially the patch breaks
> oldstyle negotiations.

It did more than that, too. Sorry 'bout the messup; I wouldn't have
released it with that bug (I never release when 'make check' fails on my
machine), but I'm not used to people actually trying to build off my git
trunk :-)

> The first problem is these lines:
> 
>  852                         if(!strcmp(p[j].paramname, "port") && 
> !strcmp(p[j].target, modernport)) {
>  853                                 g_set_error(e, errdomain, 
> CFILE_INCORRECT_PORT, "Config file specifies new-style port for oldstyle 
> export");
>  854                                 g_key_file_free(cfile);
>  855                                 return NULL;
> 
> This test will be activated whenever p[j] is equal to "port", which
> it will be in both the generic and the export section. When processing
> the generic section in an oldstyle export, there is no port directive,
> so modernport will be NULL. This will cause a SEGV, as the strcmp will
> dereference a NULL pointer.
> 
> Actually, checking here seems an odd thing to do. We could compare
> (p[j].target == &modernport) (note '&'), but that really just detects
> whether we are processing the option. We really need to see whether
> oldstyle is set, and modernport is not set (I think). That can easily
> be done at the end of the whole function (outside the loop) with:

I don't even remember what I was trying to do there. You can't specify a
newstyle port for an oldstyle export; either you're talking about the
newstyle port (which is for multiple exports), or you're talking about
the oldstyle port (which is for one export only). The two have nothing
to do with eachother.

I just junked the test, it made no sense.

>         if(modernport && do_oldstyle) {
>                 g_set_error(e, errdomain, CFILE_INCORRECT_PORT, "Config 
> file specifies new-style port for oldstyle export");
>                 g_key_file_free(cfile);
>                 return NULL;
>         }
> 
> or similar. At least I /think/ that's what it's trying to do.
> 
> Or is it trying to do this:
>    if(!strcmp(p[j].paramname, "port") && (p[j].target != &modernport) && 
> !strcmp(p[j].target, NBD_DEFAULT_PORT)) {
>     ...
>    }
> 
> (IE detect if the port option is used within the export blob but
> set 10809).

Er, ah, right, *that* was what I was trying to do: check whether you've
set the oldstyle port to whatever you'd set to the newstyle port
number, too.

That is, if you'd set 'port = 12345' in the generic section, and then
did 'port = 12345' in an export section, too, that would mean you'd be
asking nbd-server to listen to both oldstyle and newstyle exports on the
same port, which is impossible. If you hadn't set port in the generic
section, and then tried 'port = 10809' in an export section, that should
also fail.

Better put that back in, I suppose. But hey, the current trunk compiles
and passes 'make check', which is an improvement. I'll look at it later.

[...]
> Sadly, with these we lose the SEGV, but come upon a another problem.
> Essentially open_modern fails. I am not sure why open_modern is
> being called at all, with a config like:
> 
> [generic]
>         oldstyle = true
> [export]
>         exportname = /tmp/foo
>         port = 11112

If you run nbd-server with a config file, open_modern is always called.
You can't switch that off. This is by design; I want to deprecate
oldstyle exports.

(the reason that it is switched off if you run it from the command line
with no config file is that you may want to run multiple nbd-server
instances from the command line, and because you can't set a name for
the export then. But then, I want to deprecate running an export from
the command line, too)

[...]
> I'm a bit stuck. Any ideas, Wouter?

It's fixed now. Sorry for the mixup.

-- 
The volume of a pizza of thickness a and radius z can be described by
the following formula:

pi zz a