[arch-projects] [netctl] Still some weird behaviour with bridges

[Jouke Witteveen]

On Mon, May 6, 2013 at 8:03 PM, Dave Reisner <d at falconindy.com> wrote:
> On Mon, May 06, 2013 at 07:49:48PM +0200, Jouke Witteveen wrote:
>> On Mon, May 6, 2013 at 7:36 PM, Dave Reisner <d at falconindy.com> wrote:
>> > On Mon, May 06, 2013 at 07:23:58PM +0200, Jouke Witteveen wrote:
>> >> You are right. This (arrays in Bash) is pretty tricky. I believe it is
>> >> fixed now. If so, the fix will land in netctl 1.1.
>> >>
>> >> Thanks,
>> >> - Jouke
>> >
>> > The problem is that you're declaring it initially as a string, and then
>> > expecting it to be an array. You simply cannot interchange the types
>> > without encountering unwanted behavior like this.
>>
>> I'd like to keep the possibility of having declarations like this in a profile:
>>
>>   Address='1.2.3.4/5'
>>
>> Although Address is treated like an array internally, this works. As
>> far as I can tell it is relatively safe to treat non-empty strings as
>> 1-element arrays.
>
> I think this is the wrong thing to do. You need to enforce types if
> you're consuming raw bash as configuration. You're telling people that
> k='foo' is valid, but k='foo bar' is not and suddenly they must use
> k=(foo bar). This is bizzare.

It is not documented that k='foo' is valid. Accepting it is a means of
lowering user complaints.
In Bash, it is quite natural to accept strings as single element arrays:

  declare -a X
  X=foo

is a single element array, and if X is a string

  for x in "${X[@]}"; do
  ...

works as expected. Also, if X is an array, $X is its first element.

>> > Note that you're still doing this in src/netctl.in:112. I'll also point
>> > out that your -v check allows people to create profiles that do not bind
>> > to any interfaces since it checks for the definition and not the
>> > existence of a value -- are you sure you want this?
>>
>> Regarding src/netctl.in:112, the variable is set to a non-empty string
>> (in src/lib/globals:load_profile, it is actually verified that
>> $Interface is non-empty). By the above safety assumption, this should
>> be acceptable.
>> The current behavior is to bind to the Interface by default. By
>> explicitly setting BindsToInterfaces, this behavior can be altered.
>> Indeed, it is possible to do
>>
>>   BindsToInterfaces=''
>>
>> in a profile, which will most likely screw things up (it will instruct
>> systemd to bind to a device without a name). However, I consider this
>> a usage error. The current configuration allows setting
>> BindsToInterfaces to a possibly empty array as well as specifying it
>> correctly by a non-empty string. Those are the uses that I would like
>> to support. If a better way exists, I'd be happy to hear about it :-).
>
> And based on what you said above, you don't believe this is a double
> standard? If someone were to adhere to the fact that BindsToInterfaces
> is an array and not a string, then BindsToInterfaces=() is a valid way
> of declaring this as empty and netctl will respect that.

BindsToInterfaces=() is valid and meaningful. What the code doesn't do
is give an error if BindsToInterfaces (or any other variable presumed
to be an array) is not an array. There is no code to enforce
compatibility, only lack of code to enforce strictness. Line 112 could
be changed to

  [[ -v BindsToInterfaces || -z $BindsToInterfaces ]] || \
    BindsToInterfaces=( "$Interface" )

But this would change nothing in any *supported* case. In fact, the
'-z' part is debatable, because it pays attention to a situation that
is not supported anyway. Type checking is ugly in Bash and wouldn't
yield anything. Instead, all variables that are supposed to be arrays
could be declared as such a priori, which makes assigning strings to
them 'do the right thing'. In the BindsToInterfaces case, this would
then reintroduce the problems of deciding whether interface binding is
desired at all (currently: not specified = bind to Interface;
specified as empty array = no binding; otherwise: bind to all
'components')

To summarize my view:
: The current code treats arrays as arrays without type checking
: There is no code to make wacky shit possible
: As a side effect, strings as single element arrays are possible

I believe the entire discussion is only concerned with src/netctl.in:112 :-P.

- Jouke