> - Why is BasicManagementTest not a Rygel.StateMachine?

I did not even remember that one... I can test how that would work if you want, but I doubt it would be much simpler as things like BasicManagementTest.ExecutionState are still needed (to get the correct TestState return value).

> - I don't like the commented code at the end of the specific tests.
> Why not adding them to the "tests" subdir with a symlink to the
> class or something like that.

That was not supposed to be there, removed.

> - The code has several style issues, such as missing newlines before
> returns, missing spaces before and after (), function parameters,
> missing {} on single-line if's, etc.

Embarrasing, I was sure I had gone through that... Well, I've now done a sweep through the files with Rygel coding style in mind and fixed things. There were indeed lots if details there, sorry about that.

> - I'm a bit concerned about the portability, but I suppose it at least
> will compile everywhere and we can think about portability when there's
> need for that, main target platform for it should be Linux anyway, I
> suppose.

Yes, it is portable in the sense of compiling, but it will only be reliably useful on linux -- even busybox may require additional work. At least documenting this would be a good idea but I'm not sure where as it's not really relevant to most users... Is "doc/README.BasicManagement" too obscure?

> - I suppose you are going to squash the stuff?

I think that makes sense.

> - What about the TODOs in the code?

I've removed the ones that are not longer relevant. I've left a comment on task queuing as it is not crucial right now but would be if e.g. BandwidthTest implementation is added.

> Otherwise it looks fine to me.

Great.

...just so it doesn't look like I'm sneaking things in one by one,  please see bug 707848 for another additional service I've been working on.

(In reply to comment #1)
> > - Why is BasicManagementTest not a Rygel.StateMachine?
> 
> I did not even remember that one... I can test how that would work if you want,
> but I doubt it would be much simpler as things like
> BasicManagementTest.ExecutionState are still needed (to get the correct
> TestState return value).

Well it's just an abstract class that provides the async run, cancel and finished infrastructure that you have as well anyway.

> 
> > - I don't like the commented code at the end of the specific tests.
> > Why not adding them to the "tests" subdir with a symlink to the
> > class or something like that.
> 
> That was not supposed to be there, removed.

Still might be useful to put the code somewhere.

> 
> > - The code has several style issues, such as missing newlines before
> > returns, missing spaces before and after (), function parameters,
> > missing {} on single-line if's, etc.
> 
> Embarrasing, I was sure I had gone through that... Well, I've now done a sweep
> through the files with Rygel coding style in mind and fixed things. There were
> indeed lots if details there, sorry about that.

No problem :)

> 
> > - I'm a bit concerned about the portability, but I suppose it at least
> > will compile everywhere and we can think about portability when there's
> > need for that, main target platform for it should be Linux anyway, I
> > suppose.
> 
> Yes, it is portable in the sense of compiling, but it will only be reliably
> useful on linux -- even busybox may require additional work. At least
> documenting this would be a good idea but I'm not sure where as it's not really
> relevant to most users... Is "doc/README.BasicManagement" too obscure?

No, sounds fine.

Some other thing: In BasicManagementTask, you pass the void function finish_iteration to Idle.add. Are you sure this works properly and is cancelled after just one call of the function (meaning GLib assumes the function returned FALSE) ?

Damn it... I've fixed this a while ago already but there's still a weird problem with valadoc (which did not notice because I wasn't building with it): valadoc keeps segfaulting when building rygel-server docs. 

I'll try to get this and the EnergyManagement bug updated by tomorrow

Try updating to latest vala master

There's a subtle memory corruption that doesn't show everywhere and everytime, but the patch was reverted :-/

https://git.gnome.org/browse/vala/commit/?id=a09c9e93af0d64b9331c274de573465fe070b722

Created attachment 256999 [details] [review]
doc: Re-organize rygel.conf man file

Created attachment 257000 [details] [review]
core: Add UPnP BasicManagement service support

This is a squashed patch that addresses all the issues you mentioned (including using Rygel.StateMachine and the valid Idle.add issue). I've also updated the branch if you want to take a look at that instead.

About the valadoc segfault: With this patch I'm not seeing it (after making a string protected instead of private). Naturally that doesn't mean it won't appear on other machines if it really is random memory corruption in valadoc ...  But I guess the danger of that is not any higher than in other, already committed code?

Review of attachment 257000 [details] [review]:

Overall looking good, minor nitpicks

::: src/librygel-core/rygel-basic-management-test-nslookup.vala
@@ +126,3 @@
+            var builder = new StringBuilder ("");
+            foreach (var address in this.addresses) {
+                if (builder.len != 0)

missing {}

@@ +170,3 @@
+        construct {
+            this.iterations = value;
+            if (this.iterations == 0)

missing {}

@@ +292,3 @@
+
+        /* there has to be a nicer way to do this... */
+        this.results[results.length - 1] = result;

Yeah, by using a nicer data structure than an array. Also, why do you have to write that back to the array? It should be just fine to modify result or am I overlooking something? You already do this in finish_iteration, e.g.

@@ +300,3 @@
+        if (line.has_prefix ("Server:")) {
+            if (result.state != ProcessState.INIT) {
+                warning ("nslookup parser: Unexpected 'Server:' line.\n");

warning should be marked translatable

@@ +319,3 @@
+                result.addresses += line.substring ("Address:".length).strip ();
+                result.status = ResultStatus.SUCCESS;
+                if (result.answer_type == AnswerType.NONE)

missing { }

@@ +349,3 @@
+        foreach (var result in this.results) {
+            builder.append (result.to_xml_fragment ());
+            if (result.status == ResultStatus.SUCCESS)

missing {}

::: src/librygel-core/rygel-basic-management-test-traceroute.vala
@@ +176,3 @@
+            this.rtt_regex = new Regex ("(\\S+)\\s+ms\\b", 0, 0);
+        } catch (Error e) {
+            warning ("Failed to create Regexes '%s'", e.message);

The only way this could fail is that we (the programmers) provided a wrong regex, there is no user-input involved or by GLib changing its Regex internals all of a sudden. I think a hard assert_not_reached() is ok here

::: src/librygel-core/rygel-basic-management-test.vala
@@ +132,3 @@
+    private void child_setup () {
+        /* try to prevent possible changes in output */
+        Environment.set_variable("LC_MESSAGES", "C", true);

missing " " before (

@@ +145,3 @@
+        /*if we failed to initialize, skip spawning */
+        if (this.init_state != InitState.OK) {
+            Idle.add ((SourceFunc)this.finish_iteration);

I think these casts are not necessary anymore with the new finish_iteration signature.

@@ +188,3 @@
+            if (status == IOStatus.EOF) {
+                this.eof_count++;
+                if (this.eof_count > 1)

missing {}

@@ +195,3 @@
+        } catch (Error e) {
+            warning ("Failed readline() from stdout: %s", e.message);
+            this.finish_iteration();

missing " "

@@ +221,3 @@
+        } catch (Error e) {
+            warning ("Failed readline() from stderr: %s", e.message);
+            this.finish_iteration();

missing " "

@@ +237,3 @@
+        if (this.execution_state != ExecutionState.REQUESTED) {
+            warning ("Test already started");
+            return;

missing newline before return, warning should be marked translatable

@@ +239,3 @@
+            return;
+        }
+        if (this.cancellable == null)

missing {}

::: src/librygel-core/rygel-basic-management.vala
@@ +51,3 @@
+        now.tv_usec = 0;
+
+        this.device_status = "OK," + now.to_iso8601 ();

Did you check whether this is the correct iso8601 value for DLNA? We had some problems in other places with missing dashes and colons...

Review of attachment 256999 [details] [review]:

Looks good

Created attachment 257919 [details] [review]
core: Add UPnP BasicManagement service support

Fixed issues Jens pointed out (very sorry about the missing brackets which I claimed were already fixed -- shouldn't trust my grep skills too much).

I've changed some warning() calls to debug() as user would get an "internal error" in the upnp return value already. In a couple of cases I marked the string translatable as requested.

I did remember the ISO 8601 date problems: the returned format is e.g. "2013-10-23T13:26:43Z", which is fine at least according to spec.

(In reply to comment #12)

> I did remember the ISO 8601 date problems: the returned format is e.g.
> "2013-10-23T13:26:43Z", which is fine at least according to spec.

Does that generate the colon in the timezone offset or do we never get one there?
(see https://bugzilla.gnome.org/show_bug.cgi?id=702231) or is this issue not existant here?

(In reply to comment #13)
> (In reply to comment #12)
> 
> > I did remember the ISO 8601 date problems: the returned format is e.g.
> > "2013-10-23T13:26:43Z", which is fine at least according to spec.
> 
> Does that generate the colon in the timezone offset or do we never get one
> there?

There's never a timezone: the code ends up as "g_get_current_time(&now)" which does

Oops, trigger finger was too fast again:

There's never a timezone as far as I can tell: the code ends up as "g_get_current_time(&now)" which does gettimeofday() with null timezone variable.

ok

Attachment 256999 [details] pushed as d1f1466 - doc: Re-organize rygel.conf man file
Attachment 257919 [details] pushed as 9692616 - core: Add UPnP BasicManagement service support