Enhance drive specification for clearpart, ignoredisk, and partition.
 

On Thu, 2009-11-19 at 17:05 -0500, Chris Lumens wrote:
> Now you can use globs or non-device node names to specify disks in the
> arguments to these commands, which should be a big help to large storage
> configurations.
> ---
> kickstart.py | 100 ++++++++++++++++++++++++++++++++++++++--------------------
> 1 files changed, 66 insertions(+), 34 deletions(-)
>
> diff --git a/kickstart.py b/kickstart.py
> index 7735439..dd023ae 100644
> --- a/kickstart.py
> +++ b/kickstart.py
> @@ -156,6 +156,17 @@ def getEscrowCertificate(anaconda, url):
> f.close()
> return anaconda.id.escrowCertificates[url]
>
> +def deviceMatches(spec):
> + matches = udev_resolve_glob(spec)
> + dev = udev_resolve_devspec(spec)
> +
> + # udev_resolve_devspec returns None if there's no match, but we don't
> + # want that ending up in the list.
> + if dev:
> + matches.append(dev)

Shouldn't the above be something like 'if dev and dev not in matches:'
instead?

Also, about the udev_trigger... it would be nice if we could only do
that as often as is really necessary, especially for larger systems, but
that probably doesn't rank very high among our other sins.

Looks fine in general.

Dave

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Enhance drive specification for clearpart, ignoredisk, and partition.
 

Wait -- aren't you setting all of those Storage attributes that should
be lists of either Device instances or device names to instead be lists
of udev info dicts? I don't think that's gonna work.

Dave


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Enhance drive specification for clearpart, ignoredisk, and partition.
 

> > +def deviceMatches(spec):
> > + matches = udev_resolve_glob(spec)
> > + dev = udev_resolve_devspec(spec)
> > +
> > + # udev_resolve_devspec returns None if there's no match, but we don't
> > + # want that ending up in the list.
> > + if dev:
> > + matches.append(dev)
>
> Shouldn't the above be something like 'if dev and dev not in matches:'
> instead?

My thinking was that we should try both methods for resolving the spec
we're given. However, maybe there's no reason you'd ever get a glob
that's also a valid disk name so I don't need to do this.

> Also, about the udev_trigger... it would be nice if we could only do
> that as often as is really necessary, especially for larger systems, but
> that probably doesn't rank very high among our other sins.

Agreed. I wonder if we can get away with doing this only once early on.
Alternatively, we could do some sort of caching thing like so:

_triggerCache = {}
def udev_trigger(subsystem=None, action="add"):
global _triggerCache
if _triggerCache.has_key(subsystem):
return

...
iutil.execWithRedirect(...)
_triggerCache.setdefault(subsystem)

However, this is entirely too much like the old driveDict out of isys
that we always had so many problems with. I don't really like it.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Enhance drive specification for clearpart, ignoredisk, and partition.
 

> Wait -- aren't you setting all of those Storage attributes that should
> be lists of either Device instances or device names to instead be lists
> of udev info dicts? I don't think that's gonna work.

No, I don't think I am. Everything in this patch adds the return value
of udev_resolve_glob or udev_resolve_devspec to a list, and those both
appear to return names:

def udev_resolve_glob(glob):
...
for dev in udev_get_block_devices():
name = udev_device_get_name(dev)
if fnmatch.fnmatch(name, glob):
ret.append(name)
...

def udev_resolve_devspec(devspec):
...
if ret:
return udev_device_get_name(ret)

Maybe one has slipped through somewhere, though?

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Enhance drive specification for clearpart, ignoredisk, and partition.
 

On Fri, 2009-11-20 at 09:42 -0500, Chris Lumens wrote:
> > Wait -- aren't you setting all of those Storage attributes that should
> > be lists of either Device instances or device names to instead be lists
> > of udev info dicts? I don't think that's gonna work.
>
> No, I don't think I am. Everything in this patch adds the return value
> of udev_resolve_glob or udev_resolve_devspec to a list, and those both
> appear to return names:

You're right -- I was just remembering it wrong and didn't check on it.

Dave

>
> def udev_resolve_glob(glob):
> ...
> for dev in udev_get_block_devices():
> name = udev_device_get_name(dev)
> if fnmatch.fnmatch(name, glob):
> ret.append(name)
> ...
>
> def udev_resolve_devspec(devspec):
> ...
> if ret:
> return udev_device_get_name(ret)
>
> Maybe one has slipped through somewhere, though?
>
> - Chris
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@redhat.com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Enhance drive specification for clearpart, ignoredisk, and partition.
 

On 11/20/2009 09:39 AM, Chris Lumens wrote:
>>> +def deviceMatches(spec):
>>> + matches = udev_resolve_glob(spec)
>>> + dev = udev_resolve_devspec(spec)
>>> +
>>> + # udev_resolve_devspec returns None if there's no match, but we don't
>>> + # want that ending up in the list.
>>> + if dev:
>>> + matches.append(dev)
>>
>> Shouldn't the above be something like 'if dev and dev not in matches:'
>> instead?
>
> My thinking was that we should try both methods for resolving the spec
> we're given. However, maybe there's no reason you'd ever get a glob
> that's also a valid disk name so I don't need to do this.
>
>> Also, about the udev_trigger... it would be nice if we could only do
>> that as often as is really necessary, especially for larger systems, but
>> that probably doesn't rank very high among our other sins.
>
> Agreed. I wonder if we can get away with doing this only once early on.
> Alternatively, we could do some sort of caching thing like so:

Well, I think you can, for example, name an mpath device 'foo*' in kickstart
in the rhel branch (and that functionality is going to come back to master
at some point relatively soon)

But frankly, if you do that, and you're using globbing, you deserve what you
get.

--
Peter

I hope you know that this will go down on your permanent record.

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Enhance drive specification for clearpart, ignoredisk, and partition.
 

On Fri, 2009-11-20 at 09:39 -0500, Chris Lumens wrote:
> > > +def deviceMatches(spec):
> > > + matches = udev_resolve_glob(spec)
> > > + dev = udev_resolve_devspec(spec)
> > > +
> > > + # udev_resolve_devspec returns None if there's no match, but we don't
> > > + # want that ending up in the list.
> > > + if dev:
> > > + matches.append(dev)
> >
> > Shouldn't the above be something like 'if dev and dev not in matches:'
> > instead?
>
> My thinking was that we should try both methods for resolving the spec
> we're given. However, maybe there's no reason you'd ever get a glob
> that's also a valid disk name so I don't need to do this.

Makes sense.

>
> > Also, about the udev_trigger... it would be nice if we could only do
> > that as often as is really necessary, especially for larger systems, but
> > that probably doesn't rank very high among our other sins.
>
> Agreed. I wonder if we can get away with doing this only once early on.
> Alternatively, we could do some sort of caching thing like so:
>
> _triggerCache = {}
> def udev_trigger(subsystem=None, action="add"):
> global _triggerCache
> if _triggerCache.has_key(subsystem):
> return
>
> ...
> iutil.execWithRedirect(...)
> _triggerCache.setdefault(subsystem)
>
> However, this is entirely too much like the old driveDict out of isys
> that we always had so many problems with. I don't really like it.

I don't like it either. The thing is that we might want to skip the
initial trigger in storageInitialize in the kickstart case the first
time through, but we do want to do the trigger in any subsequent calls
to storageInitialize so that the database is updated before we start
looking for stuff. So it looks like, at worst, one extra call. Oh, well.

Dave

>
> - Chris
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@redhat.com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list