Re: When opening /tmp/uml.ctl fails, what do we get?
Blaisorblade <blaisorblade <at> yahoo.it>
2007-03-04 19:13:39 GMT
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
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel <at> lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
RSS Feed