Re: [Nbd] Current HEAD fails make check

[Alex Bligh <alex@...872...>]

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.

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:

       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).

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

It appears to be because setup_serve is returning
1, because serve->name is set (correctly I believe) to "export". Moreover
I don't understand why this has changed at all.

I've hit a brick wall here as I really don't understand how
setupserve is meant to work. Making open_modern use port
modernport?modernport:NBD_DEFAULT_PORT does not fix it - it just
hangs, presumably because it is terminally confused as to whether it
is doing an oldstyle or a modern negotiation.

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

--
Alex Bligh