Re: [PATCH] OpenSolaris/ZFS: C++ compatibility

On Sat, Feb 5, 2011 at 7:29 PM, Justin T. Gibbs <gibbs_at_scsiguy.com> wrote:
> On 2/5/2011 3:06 PM, Pawel Jakub Dawidek wrote:
>> On Sat, Feb 05, 2011 at 02:36:40PM -0700, Justin T. Gibbs wrote:
>>>
>>> Perhaps IllumOS will accept these changes back?  As I mentioned in the
>>> change descriptions included with the patch, the header files already
>>> show the intention of providing C++ support (extern "C" blocks), they
>>> just don't quite deliver.  The changes shouldn't be controversial.
>>
>> Sure. To be clear: I'm not against those changes, I think they are worth
>> it. And getting IllumOS to accept them back is definitely a good idea.
>
> Ok.
>
>>> We have talked internally about this at Spectra too.  Since we don't have
>>> BSD licensed nvpair code, we've thought of using Google protocol buffers
>>> to allow extensible encoding of fault data.  The GP implementation is
>>> MIT licensed and looks like it might be less cumbersome to use than
>>> nvpairs.  For the first release of our product, however, we are just
>>> making due with the string data that devctl provides.
>>
>> I've developed similar API during HAST work, maybe it is a good starting
>> point? src/sbin/hastd/nv.{c,h}.
>
> Sure.  If the decision is made to use nvpairs, I would advocate for
> emulating the Solaris API.  There's no reason to be slightly different
> from an established implementation.
>
>>> The reason I chose C++ for this task is that devd, the source of the
>>> events I process, already requires C++ so using C++ in zfsd doesn't
>>> impose any new requirements on the system.  Zfsd, like even the C
>>> kernel of FreeBSD is coded in an object oriented fashion, but its
>>> much cleaner to implement this type of design in a language that
>>> inherently supports object oriented concepts.  Could I rewrite all
>>> that I have in C?  Sure, but there would have to be some compelling
>>> reasons to offset the reduction in clarity and maintainability such
>>> a change would cause.
>>
>> Hmm, so zfsd will receive events from devd? I'm in opinion that we
>> should let devd alone. In my initial port I used devd, because it was
>> closest match, but if we want to clean it up, we shouldn't go through
>> devd. For example ZFS v28 can report whole binary blocks where checksum
>> doesn't match and passing those through devd would be cumbersome.
>
> zfsd parses devctl event data (via devd's unix domain socket) into an
> internal event class representation.  The data representation for the
> event class as well as the source for the event data can be easily
> changed out once a more complete solution is available.  For the policies
> SpectraLogic needs for its first product launch, the data coming through
> devctl is sufficient even if it is not ideal.
>
>>> Is your inability to help on a C++ version of this code due to distaste
>>> for C++ or just a lack of experience with it?
>>
>> The latter. I'm sure there are many committers that are fluent in C++,
>> but all of them know C. I was under impression that Warner implemented
>> devd in C++ also as a kind of experiment, which nobody really followed.
>
> FreeBSD has lots of examples of really well written C code and shell code,
> but almost no examples written in more modern languages (C++ isn't even
> that modern!).  That's too bad.  My hope is that, by submitting another
> example of (dare I hope well written) C++ use in FreeBSD, that this will
> change.  At least review the code (when I release it in a few weeks) before
> begging me to rewrite it! :-)

    The patch looks reasonable. It's kind of funny that the
OpenSolaris folks use variable names that conflict with reserved
keywords in C++ (class, private).
Thanks!
-Garrett