Re: [PATCH] Comm Write cleanups

On Wed, 17 Nov 2010 15:41:10 -0700, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 11/11/2010 07:55 AM, Amos Jeffries wrote:
>> Updated version of earlier patch. This one works again after the client
>> write pools merged.
>>
>> * creates namespace Comm::IO for the read and write functionality to
>> exist.
>
> Do we really need the Io sub-namespace? What Comm[unication] namespace
> parts are not about I/O? For example, all code already in src/comm/, and

> pretty much everything in comm.cc is also about I/O. If you are not
> sure, let's drop the Io namespace from Comm.

For the record comm.cc currently handles read IO, write IO, FD
management, event queues, socket opening, delay pool queueing, destination
DNS resolving. It is slightly unclear how much of that will remain in comm
layer and how much will be pushed out into better scopes.

I'm happy with removing the ::Io namespace for now. Will do before commit.

>
>> +}; // namespace Io
>> +}; // namespace Comm
>
> Extra semicolon after '}'.
>
> You may want to fix others as well but such changes would be out of this

> patch scope, of course:
>
> src/auth/Config.h:}; // namespace Auth
> src/comm/ListenStateData.h:}; // namespace Comm
> src/esi/Module.h:}; // namespace Esi
> src/eui/Config.h:}; // namespace Eui
> src/eui/Eui48.h:}; // namespace Eui
> src/eui/Eui64.h:}; // namespace Eui
> src/fs/Module.h:}; // namespace Fs
> src/ident/Config.h:}; // namespace Ident
> src/ident/Ident.cc:}; // namespace Ident
> src/ip/Address.h:}; // namespace Ip
> src/ip/Intercept.h:}; // namespace Ip
> src/ip/QosConfig.h:}; // namespace Qos
> src/ip/QosConfig.h:}; // namespace Ip
> src/ip/tools.h:}; // namespace Ip
> src/ipc/StrandCoords.h:}; // namespace Ipc
> src/log/Config.h:}; // namespace Log
> src/SquidMath.h:}; // namespace Math
>
>
>> * The comm_write() functions are moved into that scope as
>> Comm::IO::Write() and only accept AsyncCall now.
>>
>> * commio_* functions are all moved to methods of a new
>> Comm::Io::Callback object. Which represents either a read or a write
>> callback event waiting to happen. Old wrapper versions of the functions
>> have been removed.
>>
>> * The fdc_table of pending read and write callbacks has been moved into
>> the Comm::IO scope with (the name iocb_table) and should be considered
>> private. For now the COMMIO_*_CB() macros are retained to produce a
>> pointer to a callback object in this table.
>>
>> * libcomm-listener.la has been renamed to libcomm.la
>
> * Should comm_err_t and its members be moved inside Comm namespace,
> dropping their comm prefixes? We can use transitional #defines to avoid
> widespread changes if that is what you are trying to prevent. It is kind

> of wrong to move comm_err_t to comm/ but leave it outside the Comm
> namespace.

comm_err_t should indeed be moved into the namespace, however its used
very widely and the symbol change does not quite fit with this patch.
I was also using it with the strings converter awk script to pretty-print
debugs() in earlier versions. The awk script can't handle namespacing.

moving it back out to src/comm_err_t.h until that followup cleanup is
done.

>
>
>> This is not expected to produce any functionality changes.
>
> Overall, this looks good to me although I could not verify that there
> are no functionality changes due to code relocation (I have heard that
> only git can track those).

Ooh way to go git! I thought that kind of proofing died with Z language.
I was careful to cut-n-paste by function then only change symbol names.
Hoping.

Since these are few and all cosmetic is this a +1 from you?

Amos
Received on Wed Nov 17 2010 - 23:12:16 MST