Re: Module problem on amd64

[Paul Goyette <paul%whooppee.com@localhost>]

On Sun, 30 Nov 2008, Andrew Doran wrote:

> On Fri, Nov 28, 2008 at 04:54:46PM -0800, Paul Goyette wrote:
>
> > Well, even if noone else thinks it is useful, I went and did the deed!
>
> Cool, thanks! I think there is MD code that plays with 'mountroot'. See:
>
> http://nxr.homeunix.org/source/search?q=&defs=&refs=mountroot&path=&hist=
>
> > TEST1 and TEST2 boot successfully to single user, and I'm able to do a
> > 'ls' command from the single-user shell prompt.  However, both of these
> > panic due to the "end <= VM_MAX_KERNEL_ADDRESS" thing mentioned in an
> > earlier thread.
>
> I will fix that soon.
>
> > +               /*
> > +                * If user specified a file system type, use only
> > +                * that type
> > +                */
> > +               if (strcmp(rootfstype, "?") && strcmp(rootfstype, v->vfs_name))
> > +                       continue;
>
> KNF says 'strcmp(a, b) == 0'

Well, hopefully you meant 'strcmp(a,b) != 0' ??  I have fixed this.

> > +       static char my_vfs_name[NAME_MAX + 1];
>
> Should be MNAMELEN from mount.h, I think.

OK, that's easy to fix.

> > +                               /*
> > +                                * Copy the vfs_name since *vops might be
> > +                                * deallocated?
> > +                                */
> > +                               strncpy(my_vfs_name, vops->vfs_name,
> > +                                       sizeof(my_vfs_name));
> > +                               rootfstype = (const char *)my_vfs_name;
> >                                 vfs_delref(vops);
> >                                 break;
>
> Can you make 'rootfstype' in ioconf.c (or wherever it is emitted) into an
> array sized MNAMELEN and not a pointer? That way, we can get rid of
> 'mountroot' completely and always key on the name. So the code in
> vfs_mountroot() would look like:

It's emitted in config's mkswap.c, and defined in sys/systm.h.

I've already completely removed the mountroot variable;  everything 
already keys only on the name.  The only benefit of allocating the new 
rootfstype as a fixed array is to avoid having the local 'my_vfs_name' 
above.

But, if I define it as an array in swapnetbsd.c, it would also need to 
be defined that way is sys/sys/systm.h, and then either systm.h would 
need to #include mount.h, or all of its users would need to do that.

So, which is more desirable?

        1. #include <mount.h> in systm.h?
        2. #include <mount.h> in all users of systm.h?
        3. Have my_vfs_name statically allocated in kern_subr.c, retain
           rootfstype as a simple pointer, and make rootfstype point to
           it if needed?

>         if (strcmp(rootfstype, "?") != 0 &&
>             (v = vfs_getopsbyname(rootfstype)) != NULL) {
>                 if (v->vfs_mountroot != NULL) {
>                         error = (*(v->vfs_mountroot))();
>                         vfs_delref(v);
>                         goto done;
>                 } else {
>                         vfs_delref(v);
>                 }
>         }
>
> I have just noticed a bug in the existing code. In setroot():
>
> http://nxr.homeunix.org/source/xref/sys/kern/kern_subr.c#928
>
> If vops->vfs_mountroot == NULL, it should do vfs_delref(vops) before
> continuing on.

I've added the additional call to vfs_delref() as follows:

                        vops = vfs_getopsbyname(buf);
                        if (vops == NULL || vops->vfs_mountroot == NULL) {
                                printf("use one of: generic");
                                for (vops = LIST_FIRST(&vfs_list);
                                     vops != NULL;
                                     vops = LIST_NEXT(vops, vfs_list)) {
                                        if (vops->vfs_mountroot != NULL)
                                                printf(" %s", vops->vfs_name);
                                }
                                if (vops != NULL)
                                        vfs_delref(vops);
#if defined(DDB)
                                printf(" ddb");
#endif
                                printf(" halt reboot\n");
                        } else {


I'm going to add another test kernel to force me into the RB_ASKNAME 
parts of setroot() just to verify that that code path works.

----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul%whooppee.com@localhost   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette%juniper.net@localhost |
----------------------------------------------------------------------