On Mon, 2013-05-20 at 11:39 -0700, Andy Grover wrote: > On 05/17/2013 06:35 PM, Nicholas A. Bellinger wrote: > > No, it's the pulling out of aptpl metadata from configfs at random times > > that can lead to incorrect state. Just having that bit exposed in patch > > #12 doesn't make any sense given the NAK on the original #13, which is > > why I'm not applying it either. > > Why would reading aptpl metadata from configfs lead to incorrect state? > Again, look closer at the code. ALL_TG_PT is one example where someone randomly reading aptpl metadata from configfs can get the incorrect state, but there are certainly more cases than that. There's a reason why aptpl updates are currently performed at specific times in the PR logic path, and it's an incorrect assumption that they can be read at any time from user-space and get the correct state for all registrations and reservations. > > Huh..? kernel_write translates to vfs_write.. So your saying that > > these are going away long-term..? > > My example was crappy. vfs_write has its uses -- I have no problem with > fileio backstore using it -- but basically the kernel should not (and > generally does not) write configuration info to the filesystem; config > is pushed down into the kernel by userspace, and read by userspace via > mechanisms besides the kernel writing files. > > >> Policy should not go in the kernel. > > > > Nonsense, this has nothing to do with policy. > > Here's an excerpt from an article GregKH (CCd) wrote: > > (talks about reading config from fs but also applies to writing) > > http://www.linuxjournal.com/article/8110 > > "Trying to protect the kernel from dumb programming errors is not the > most important reason for not allowing drivers to read files. The > biggest issue is policy. Linux kernel programmers try to flee from the > word policy as fast as they can. They almost never want to force the > kernel to force a policy on to user space that can possibly be avoided. > Having a module read a file from a filesystem at a specific location > forces the policy of the location of that file to be set. If a Linux > distributor decides the easiest way to handle all configuration files > for the system is to place them in the /var/black/hole/of/configs, this > kernel module has to be modified to support this change. This is > unacceptable to the Linux kernel community." > The only policy is the the hardcoded PR aptpl metadata path, and like I said before, go ahead and submit a patch to allow the changing of the path. Aside from that, and given PR aptpl metadata is not being *read* by kernel-space, your argument for an upcall and user-space dependency in an existing I/O path is extremely thin. > > > Whatever you want to do can be done with the existing code using inotify > > on the metadata files in question, with the exact same formatting as the > > changes you've already proposed. > > Yuck. Run a daemon?? > > We're using configfs for everything else, so we just need to make this > also available there, and then poke userspace when needed to save it and > guard against unexpected reboots. call_usermodehelper() seems like the > right approach. > The more I think about this, the less and less sense it makes any sense to me. Why you think that exporting possibly dozens of key=value formatted PR registrations via a single configfs attribute is OK is beyond me, but it certainly breaks every rule about keeping a single value for sysfs / configfs attribute output that I've heard about. --nab -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
