genodelabs / goa

Tool for streamlining the development of Genode applications
GNU Affero General Public License v3.0
19 stars 17 forks source link

Support runtime requirements for <nic> and <file_system> #6

Closed chelmuth closed 4 years ago

nfeske commented 4 years ago

Very cool! This opens up so many new usage scenarios.

I think that the name of the file-system directory as stated in the commit message of https://github.com/nfeske/goa/pull/6/commits/8458fa4b525a12a8d80493c9e8edc74447a7bef1 is outdated. According to the patch, it is var/fs/<label>.

I'd prefer to live without the extra dependency from xmlstarlet. Granted, xmllint is quite hard to use but it's worth going the extra mile to keep the goa dependencies small. @chelmuth if you feel repulsed, I might give it a try.

chelmuth commented 4 years ago

I updated the commit message and would love to see your take on node selection without xmlstarlet as I failed to implement it in reasonable time.

nfeske commented 4 years ago

I will give it a try then.

nfeske commented 4 years ago

What do you think of the idea of the following Tcl snippet?

set runtime_file repos/ports/recipes/pkg/noux-system/runtime

# obtain amount of <file_system> nodes
set num_fs [exec xmllint --xpath "count(/runtime/requires/file_system)"  $runtime_file]

##
# Request attribute value from nth <file_system> node
#
proc file_system_attr { runtime_file n attr_name default_value } {

    set value [exec xmllint \
                    --xpath "string(/runtime/requires/file_system\[$n\]/@$attr_name)" \
                    $runtime_file]
    if {$value != ""} {
        return $value }

    return $default_value
}

# iterate over <file_system> nodes
for {set i 1} {$i <= $num_fs} {incr i} {

    set label     [file_system_attr $runtime_file $i "label" "default"]
    set writeable [file_system_attr $runtime_file $i "writeable" "no"]

    puts "file system $i has label=$label writeable=$writeable"
}
chelmuth commented 4 years ago

This should work. How do you think about the diagnostic message about missing label attributes in my original patch? Could be added like follows.

# iterate over <file_system> nodes
for {set i 1} {$i <= $num_fs} {incr i} {

    set label     [file_system_attr $runtime_file $i "label"     ""]
    set writeable [file_system_attr $runtime_file $i "writeable" "no"]

    if {$label == ""} {
        puts "warning: file systems without labels will be ignored"
    } else {
        puts "file system $i has label=$label writeable=$writeable"
    }
}

Tested with the following runtime file.

<runtime>
    <requires>
        <file_system/>
        <file_system                           />
        <file_system label=""                  />
        <file_system label="x"                 />
        <file_system            writeable=""   />
        <file_system label=""   writeable=""   />
        <file_system label="x"  writeable=""   />
        <file_system label="x"  writeable="yes"/>
    </requires>
</runtime>

It goes without saying that I'm ready to integrate the idea ;-)

nfeske commented 4 years ago

My code snippet was just a sketch to try xmllint as alternative to xmlstarlet.

Of course, I completely support including the diagnostic warning whenever the label is missing. I'd probably drop the good-case message though.

It goes without saying that I'm looking forward for your new patch. :-)

nfeske commented 4 years ago

BTW, for printing diagnostic warnings, it is a good practice to target stderr:

puts stderr "Warning: file systems without labels will be ignored"

Within Goa, I use to write "warning:" as "Warning:".

chelmuth commented 4 years ago

In the original patch I used proc log as it prefixes the message with the project file, but this proc does not use stderr.

nfeske commented 4 years ago

In the original patch I used proc log as it prefixes the message with the project file, but this proc does not use stderr.

The log procedure wasn't meant for warnings and errors, just for regular non-error output. But it is rarely used because Goa tries to be as quiet as possible. In fact, when looking at the code, I see it used only to print the "exported " messages, which are really of interest for the user. Most other messages of the kind "doing this, doing that" aren't.

There is also the diag procedure, which can be used to print informative messages that are shown in verbose mode.

chelmuth commented 4 years ago

Removed the xmlstarlet dependency and updated the topic branch. Works with the procedure described at https://genodians.org/chelmuth/2020-03-16-lets-encrypt and the test file above.

nfeske commented 4 years ago

Perfect! Thank you for the adjustments. I merged both commits.