FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Kubuntu Development

 
 
LinkBack Thread Tools
 
Old 08-10-2011, 07:48 PM
Chris Lumens
 
Default Carry rdloaddriver= parameters through to the boot loader config.

> Handle multiple rdloaddriver= parameters in the cmdlineDict. In cases
> where we already have a key, convert the value to a set and add the new
> value to it. Start new keys with just the value.
>
> In the boot loader code, take cmdlineDict values that are sets, convert
> them to lists, and join them to a single string separated by commas
> before writing them to the argument list.

This set looks fine, except for comments below.

> @@ -174,7 +174,10 @@ class KernelArguments:
>
> val = flags.cmdline.get(arg, "")
> if val:
> - newArgs.append("%s=%s" % (arg, val))
> + if type(val) == type(set()):
> + newArgs.append("%s=%s" % (arg, ','.join(list(val)),))
> + else:
> + newArgs.append("%s=%s" % (arg, val,))
> else:
> newArgs.append(arg)
>

You don't need the trailing commas in your tuples. The only time you
need to use a trailing comma is if you're creating a singleton tuple
because python syntax is a little weird there. Makes it a little easier
to read, I think.

> diff --git a/flags.py b/flags.py
> index e01ec63..afb56e2 100644
> --- a/flags.py
> +++ b/flags.py
> @@ -61,7 +61,19 @@ class Flags:
> key = i
> val = None
>
> - cmdlineDict[key] = val
> + if key.lower() == "rdloaddriver":
> + key = key.lower()
> +
> + if cmdlineDict.has_key(key):
> + if type(cmdlineDict[key]) == type(set()):
> + cmdlineDict[key].add(val)
> + else:
> + tmpset = set()
> + tmpset.add(cmdlineDict[key])
> + tmpset.add(val)
> + cmdlineDict[key] = tmpset
> + else:
> + cmdlineDict[key] = val
>
> return cmdlineDict

I don't really like this, but I think it will be okay. We don't appear
to allow for multiple instances of a command line option anywhere else
in anaconda, so you shouldn't be breaking existing behavior by using a
set when that comes up.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 08-10-2011, 07:54 PM
David Cantrell
 
Default Carry rdloaddriver= parameters through to the boot loader config.

On 08/10/2011 03:48 PM, Chris Lumens wrote:

Handle multiple rdloaddriver= parameters in the cmdlineDict. In cases
where we already have a key, convert the value to a set and add the new
value to it. Start new keys with just the value.

In the boot loader code, take cmdlineDict values that are sets, convert
them to lists, and join them to a single string separated by commas
before writing them to the argument list.


This set looks fine, except for comments below.


@@ -174,7 +174,10 @@ class KernelArguments:

val = flags.cmdline.get(arg, "")
if val:
- newArgs.append("%s=%s" % (arg, val))
+ if type(val) == type(set()):
+ newArgs.append("%s=%s" % (arg, ','.join(list(val)),))
+ else:
+ newArgs.append("%s=%s" % (arg, val,))
else:
newArgs.append(arg)



You don't need the trailing commas in your tuples. The only time you
need to use a trailing comma is if you're creating a singleton tuple
because python syntax is a little weird there. Makes it a little easier
to read, I think.


Changed. I won't post a new patch since it's removing two commas.


diff --git a/flags.py b/flags.py
index e01ec63..afb56e2 100644
--- a/flags.py
+++ b/flags.py
@@ -61,7 +61,19 @@ class Flags:
key = i
val = None

- cmdlineDict[key] = val
+ if key.lower() == "rdloaddriver":
+ key = key.lower()
+
+ if cmdlineDict.has_key(key):
+ if type(cmdlineDict[key]) == type(set()):
+ cmdlineDict[key].add(val)
+ else:
+ tmpset = set()
+ tmpset.add(cmdlineDict[key])
+ tmpset.add(val)
+ cmdlineDict[key] = tmpset
+ else:
+ cmdlineDict[key] = val

return cmdlineDict


I don't really like this, but I think it will be okay. We don't appear
to allow for multiple instances of a command line option anywhere else
in anaconda, so you shouldn't be breaking existing behavior by using a
set when that comes up.


I'll wait to hear from the rest of the team too. I'm not too worried
about it, but tend to be thinking about this change the same way you are.


--
David Cantrell <dcantrell@redhat.com>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 08-11-2011, 12:05 AM
"Brian C. Lane"
 
Default Carry rdloaddriver= parameters through to the boot loader config.

On Wed, Aug 10, 2011 at 03:54:04PM -0400, David Cantrell wrote:
> On 08/10/2011 03:48 PM, Chris Lumens wrote:

[snip]

> >
> >I don't really like this, but I think it will be okay. We don't appear
> >to allow for multiple instances of a command line option anywhere else
> >in anaconda, so you shouldn't be breaking existing behavior by using a
> >set when that comes up.
>
> I'll wait to hear from the rest of the team too. I'm not too
> worried about it, but tend to be thinking about this change the same
> way you are.

rdloaddriver supports comma separated modules so I don't see any need to
support multiple instances of it -- especially since that will encourage
users to think other commands could do the same.

--
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 08-11-2011, 09:26 AM
Radek Vykydal
 
Default Carry rdloaddriver= parameters through to the boot loader config.

On 08/11/2011 02:05 AM, Brian C. Lane wrote:

On Wed, Aug 10, 2011 at 03:54:04PM -0400, David Cantrell wrote:

On 08/10/2011 03:48 PM, Chris Lumens wrote:

[snip]


I don't really like this, but I think it will be okay. We don't appear
to allow for multiple instances of a command line option anywhere else
in anaconda, so you shouldn't be breaking existing behavior by using a
set when that comes up.

I'll wait to hear from the rest of the team too. I'm not too
worried about it, but tend to be thinking about this change the same
way you are.

rdloaddriver supports comma separated modules so I don't see any need to
support multiple instances of it -- especially since that will encourage
users to think other commands could do the same.



+1. The only things that I can think of as reasons for complicating
boot options handling is the comfort of adding a module with another
instance of the option, or dracut allowing the same, but I am not
sure it is worth it. Or are there any others?

Radek

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 08-11-2011, 01:57 PM
David Cantrell
 
Default Carry rdloaddriver= parameters through to the boot loader config.

On 08/11/2011 05:26 AM, Radek Vykydal wrote:

On 08/11/2011 02:05 AM, Brian C. Lane wrote:

On Wed, Aug 10, 2011 at 03:54:04PM -0400, David Cantrell wrote:

On 08/10/2011 03:48 PM, Chris Lumens wrote:

[snip]


I don't really like this, but I think it will be okay. We don't appear
to allow for multiple instances of a command line option anywhere else
in anaconda, so you shouldn't be breaking existing behavior by using a
set when that comes up.

I'll wait to hear from the rest of the team too. I'm not too
worried about it, but tend to be thinking about this change the same
way you are.

rdloaddriver supports comma separated modules so I don't see any need to
support multiple instances of it -- especially since that will encourage
users to think other commands could do the same.



+1. The only things that I can think of as reasons for complicating
boot options handling is the comfort of adding a module with another
instance of the option, or dracut allowing the same, but I am not
sure it is worth it. Or are there any others?


Yeah, I think I'm going to go with the more simple approach. The reason
I was supporting multiple rdloaddriver= parameters is because dracut
does and I was aiming for support like dracut. But yeah, there's really
no point in our case. So long as we at least support rdloaddriver=,
we're good.


New patches coming up. Thanks for the feedback.

--
David Cantrell <dcantrell@redhat.com>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT

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

Thread Tools




All times are GMT. The time now is 04:48 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org