Re: [gentoo-portage-dev] [PATCH] Check for and report read-only filesystems

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: Text/Plain, Size: 1718 bytes --]

On Friday 10 January 2014 22:07:52 Chris Reffett wrote:
> Attached is a patch to test if Portage is going to write to a
> read-only filesystem and print out the list of filesystems that need
> to be remounted RW. This leaves ${D} intact rather than having some
> files moved before hitting the RO filesystem. Fixes bug 378869. Since
> git.overlays.gentoo.org is down, I haven't had the chance to rebase
> this against latest, but I can resubmit if it doesn't cleanly apply.
> This is my first patch to the list, so I apologize if I didn't submit
> correctly.

please use `git send-email` to post patches.  attaching them makes things 
harder to review.

> --- /dev/null
> +++ b/pym/portage/util/checkwriteable.py
>
> +		with open("/proc/mounts") as procmounts:

this won't work on non-Linux systems

also, as a general style thing, unless there's a real need for the var to have 
a "full" name, just use "f" or "fp"

> +			roregex = re.compile(r'(\A|,)ro(\Z|,)')
> +			for line in procmounts:
> +				if roregex.search(line.split(" ")[3].strip()) is not None:
> +					romount = line.split(" ")[1].strip()
> +					ro_filesystems.add(romount)

ad hoc parsing of /proc/mounts isn't a good thing.  at the risk of over 
engineering, there should be a func that would take care of expanding the 
mount paths into a list of namedcollections (one per mount).  then you simply 
walk it looking at its options.

> +	for directory in dir_list:
> +		for filesystem in ro_filesystems:
> +			if re.match(filesystem, directory):
> +				ro_filesystems_written.add(filesystem)

there's no need to use re and in fact you don't want to.  you're looking at 
wrong strings here, not regexes.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]