Zeroconf cpp/lzp options patch

[Ihor Kaharlichenko]

Greetings.

Could anybody take a look at patch attached to
http://code.google.com/p/distcc/issues/detail?id=59?

It makes distcc --show-hosts show cpp and lzo options in host
descriptions found by means of Zeroconf. It in turn makes it possible
to use those hosts in pump mode.

[Fergus Henderson]

Thanks for the patch!

Generally that looks good.

One potential issue is that it will _always_ enable ",cpp,lzo" for zeroconf hosts,

which will cause some problems for people who are _not_ using pump mode.

It might be better to change the syntax of the hosts file to allow "+zeroconf,cpp,lzo" in the hosts file,

and to filter the options found via DNS-SD with the options from the client's hosts list

(i.e. take the intersection of the two option lists).

Then users who want to use pump mode can use "+zeroconf,cpp,lzo" in their hosts file,

and users who just want to use zeroconf without pump mode or compression can use "+zeroconf" in their hosts file.

Still, I think your patch is already an improvement on the status quo.

Some small suggestions:

+        char t[256], o[10], a[AVAHI_ADDRESS_STR_MAX]; 

  - Despite the poor examples in surrounding code,

    I suggest using a longer variable name, e.g. 'options', rather than 'o'.

  - s/10/sizeof(",cpp,lzo")/

+            strncat(o, ",cpp", sizeof(o));

  - strncat() doesn't guarantee to null-terminate the string.

    In this case, you don't need to use strncat() - you can just use strcat().

It would be nice to add a test case to test your change, or indeed to have any test of the zeroconf support at all

(current there are no tests for that).

Also the distcc manual (man/distcc.1) has a a sentence about this:

An important caveat is that in the current implementation,

pump mode (",cpp") and compression (",lzo") will never be

used for hosts located via zeroconf.

That should be updated (or perhaps just deleted).

Would you like to revise the patch to address these review comments?

Cheers,

  Fergus.
--
Fergus Henderson <fer...@google.com>

[Ihor Kaharlichenko]

> One potential issue is that it will _always_ enable ",cpp,lzo" for zeroconf
> hosts,
> which will cause some problems for people who are _not_ using pump mode.
> It might be better to change the syntax of the hosts file to allow
> "+zeroconf,cpp,lzo" in the hosts file,
> and to filter the options found via DNS-SD with the options from the
> client's hosts list
> (i.e. take the intersection of the two option lists).
> Then users who want to use pump mode can use "+zeroconf,cpp,lzo" in their
> hosts file,
> and users who just want to use zeroconf without pump mode or compression can
> use "+zeroconf" in their hosts file.

To be honest it took me quite a while until I got what did you mean by
saying that :) But eventually I made it! I guess this kind of change
should also be reflected in documentation.

> +        char t[256], o[10], a[AVAHI_ADDRESS_STR_MAX];
>
>   - Despite the poor examples in surrounding code,
>     I suggest using a longer variable name, e.g. 'options', rather than 'o'.

Actually I prefer descriptive variable names myself but I tried to
stick to the coding style of yours, thus "o" instead of "options".
Well, it's not a problem to fix that.

>   - s/10/sizeof(",cpp,lzo")/
>
> +            strncat(o, ",cpp", sizeof(o));
>
>   - strncat() doesn't guarantee to null-terminate the string.
>     In this case, you don't need to use strncat() - you can just use
> strcat().

Thanks for pointing that out. I'm from C++/Java world and not that
good at C-way of handling strings.

> It would be nice to add a test case to test your change, or indeed to have
> any test of the zeroconf support at all
> (current there are no tests for that).

I'd be glad to do that although I faced some problems while studying
distcc's testing stuff (see http://code.google.com/p/distcc/issues/detail?id=60
and http://code.google.com/p/distcc/issues/detail?id=61). I'd love to
write a couple of tests related to Zeroconf if you could solve them
and point me out what can be tested in this situation.

> Also the distcc manual (man/distcc.1) has a a sentence about this:
>
> An important caveat is that in the current implementation,
> pump mode (",cpp") and compression (",lzo") will never be
> used for hosts located via zeroconf.
>
> That should be updated (or perhaps just deleted).

I'll take care of it as soon as I add .

> Would you like to revise the patch to address these review comments?

I'll gladly fix all the issues you've mentioned. I hope I can come
back with some results in a couple of days.

Regards, Ihor.