Re: [PATCH 1/2] Fix some minor issues.

[Sergiu Ivanov]

Hello,

On Wed, Aug 12, 2009 at 02:42:59PM +0200, olafBuddenhagen@gmx.net wrote:
> On Mon, Aug 10, 2009 at 11:11:41PM +0300, Sergiu Ivanov wrote:
> 
> > * eth-multiplexer/device_impl.c (ds_device_open): Check if
> > openstat flags are a subset of mode flags (not conversely).
> > * eth-multiplexer/vdev.c (add_vdev): Copy name only if it
> > is not NULL.
> 
> Why are you putting these two changes in one patch? They seem rather
> orthogonal.

Yep, they are.  However (as it might be derived from the one-line
description), this patch is a kind of clean-up patch; that is why I
put two different changes in the same patch.

Nevertheless, I guess you should read this mail to its end to see how
I gradually come to the conclusion that this patch should be
fundamentally modified :-(
 
> > While the second fix is obvious, I cannot explain for sure why do we
> > need the first fix.  What I can tell exactly is that without it the
> > .MASTER node does not work.  I think I will have to investigate into
> > this issue further in the future.
> 
> Don't make changes you do not understand...

Well, what I *understood* about this change is that with it things
worked and without it things didn't work :-)

However, now I'm ready to explain why this change is required (I
guess, I should have tried to understand it earlier).  I'll put the
explanation under your next comment, since I'll need to refer to
variables.

> > diff --git a/eth-multiplexer/device_impl.c b/eth-multiplexer/device_impl.c
> > index f9c8fc3..4b2d37d 100644
> > --- a/eth-multiplexer/device_impl.c
> > +++ b/eth-multiplexer/device_impl.c
> > @@ -122,9 +122,9 @@ ds_device_open (mach_port_t master_port, mach_port_t 
> > reply_port,
> >    dev = (struct vether_device *) pi->po->np->nn->ln;
> >    /* check the mode */
> >    openstat = pi->po->openstat;
> > -  if (mode & D_READ && !(openstat & O_READ))
> > +  if ((openstat & O_READ) && !(mode & D_READ))
> >      right_mode = 0;
> > -  if (mode & D_WRITE && !(openstat & O_WRITE))
> > +  if ((openstat & O_WRITE) && !(mode & D_WRITE))
> 
> The only actual change here seems to be the added parenthesis? Or am I
> missing something?...

It's not really like that :-)

This change affects only static devnode instances.  Take a look at
hurd/devnode/devnode.c:278 (parse_opt): devnode opens the node
provided via ``-M'' with flags = 0.  Now let's get back to the code
region modified in this diff.  `openstat` contains the flags with
which the libnetfs node was opened, while `mode` contains the flags
for opening the device.  In the case of static instances of devnode,
openstat will *always* be 0 (due to the way devnode handles the ``-M''
option), while `mode` will, obviously, take nonzero values in most of
the cases.

In the initial version of this code region, Zheng checked for flags in
`mode` *first*, which required `mode` to be a subset of `openstat`.
My change turns it the other way round: `openstat` is required to be a
subset of `mode`.  This opens the way for static `devnode` instances
to work.

The important conclusions for me are: 1. Don't make changes I don't
understand ( ;-) ) 2. Move this change into the patch introducing the
.MASTER node, since this change is only related to this situation.

Also, I'm thinking whether it won't be better to leave the check for
flags as it was in the initial version and just skip this check when
the device is opened via the ``.MASTER'' node.  What do you think?
 
> > diff --git a/eth-multiplexer/vdev.c b/eth-multiplexer/vdev.c
> > index dac9802..6fb88d0 100644
> > --- a/eth-multiplexer/vdev.c
> > +++ b/eth-multiplexer/vdev.c
> > @@ -135,7 +135,10 @@ add_vdev (char *name, int size,
> >  
> >    vdev->dev_port = ports_get_right (vdev);
> >    ports_port_deref (vdev);
> > -  strncpy (vdev->name, name, IFNAMSIZ);
> > +  if (name)
> > +    strncpy (vdev->name, name, IFNAMSIZ);
> > +  else
> > +    vdev->name[0] = 0;
> 
> I assume the NULL case happens for the root node only?

Yep.
 
> Is this really useful?...

Well, I'd generally opt for keeping such an elementary check for
memory validity, although it might not be necessary ATM.  I don't
consider it YAGNI, because it's a very basic means of protection.

Also, please note that, while being unnecessary under normal
circumstances, this check will prevent the multiplexer from crashing
when somebody tries opening a device with a NULL name (via .MASTER,
for instance).  It might even be suitable to return something like
EINVAL in case the name is null.  What do you think?

As a general conclusion I'd suggest the following modifications:

1. Remove the change of flag checking policy; instead just don't check
   flags when device creation is requested via .MASTER.

2. Rename this patch to ``Handle NULL device names'' and make add_vdev
   return EINVAL when a null device name is requested.

What do you think?

Regards,
scolobb