Re: [uml-devel] When opening /tmp/uml.ctl fails, what do we get?

[Blaisorblade <bla...@ya...>]

On Thursday 22 February 2007 20:59, Jeff Dike wrote:
> On Thu, Feb 22, 2007 at 12:54:04AM +0100, Blaisorblade wrote:
> > I started uml_daemon as root by mistake, so /tmp/uml.ctl even if
> > /dev/net/tun was world-readable. Ok, I did it. This is the result (after
> > trying many times to do 'ifconfig eth0 up' inside UML):
>
> I don't see your irq stuff here when I try that, but there were
> multiple problems, which I hope are fixed by the patch below:
> 	style violations - this made the patch much bigger, and needs
> to be split out

Indeed, sigh. However I'm now convinced that if this just causes delays in 
patch merging without review benefit, it's not a good thing. It should be 
easy to split this away, and I have the patch about errno and am going to 
merge it (I hope to give a flush to my patch queue tomorrow).

However you still add new ones:

+       if(platform_device_register(&device->pdev))
+               goto out_free_netdev;

> 	the user init routines now return success or failure

Good.

> 	some major cleaning of the failure paths through eth_configure
> 		device was never freed
> 		sysfs_unregister was never called, adding the same
> interface again successfully would result in nasty error messages
> 		device stuck in the devices list regardless, so adding
> the same interface again would fail
> 		the return from platform_device_register was not
> checked

> 		I moved the register_netdev call until almost the end
> of the function

Have you checked if other calls require that register_netdev was already 
called?

> Let me know what you think about this.
>
> And about your thought of merging the _init into _open, my excuse for
> keeping them separate deserves some scrutiny.  Every transport besides
> the switch does all of its work in _open, and the _init routine just
> initializes data.  So, I would say that if a transport has separate
> plug-it-in and bring-it-up operations, maybe they should be separated
> more than they are now.  If the switch is the odd one, maybe we should
> merge _init into _open.

Hmm, I'm really not sure about what to do about this. However I'd leave that 
for a future cleanup and just add a comment about this.
And the reason for which init now returns a value is that pcap_init _can_ 
fail, so the switch transport is not the only odd one.

Another note:
+       /*
+        * This overallocates by sizeof(int) since the last field in
+        * uml_net_private is used to access the transport-specific
+        * data, so that user field is the first 4 bytes of the
+        * transport data.
+        */
+       size = transport->private_size + sizeof(struct uml_net_private);

There is a simple and standard solution in Linux style:

struct uml_net_private {
-        int user[1];
+        char user[0];
};

With fgrep [0] include/linux/* I found quite a few examples (like in 
include/linux/ethtool.h) - even if it is not fully ISO C conforming it's gcc 
conforming.

A proof-of-concept (i.e. untested and applying on vanilla) patch for this is 
attached, with name remember-zero-array-net.diff.

Another note: before of this patch, you should please apply the attached two 
ones.

*) The first (net-mac-check-cleanup.diff) checks the validity of assigned MAC 
address, but to print a meaningful error message requires adding a local 
buffer.
*) The second (net_kern-eth_configure...) allows avoiding this local buffer by 
moving code around.

I think I'm excessively paranoid about these two patches and about not yet 
merging them, so please give a look and merge them.
-- 
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade