Date: Sat, 5 Feb 2011 20:43:51 -0800 From: Garrett Cooper <gcooper@FreeBSD.org> To: gibbs@scsiguy.com Cc: Pawel Jakub Dawidek <pjd@freebsd.org>, current@freebsd.org Subject: Re: [PATCH] OpenSolaris/ZFS: C++ compatibility Message-ID: <AANLkTimg33WYkuq7bTe=obTsxJdVV8Sat5THdL9E3Mu6@mail.gmail.com> In-Reply-To: <4D4E1585.3030709@scsiguy.com> References: <4D4C3F89.50700@scsiguy.com> <20110205153920.GS2035@garage.freebsd.pl> <4D4DC2E8.60901@scsiguy.com> <20110205220646.GZ2035@garage.freebsd.pl> <4D4E1585.3030709@scsiguy.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Feb 5, 2011 at 7:29 PM, Justin T. Gibbs <gibbs@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? =A0As I mentioned in th=
e
>>> 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. =A0The 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. =A0Since we don't =
have
>>> BSD licensed nvpair code, we've thought of using Google protocol buffer=
s
>>> to allow extensible encoding of fault data. =A0The GP implementation is
>>> MIT licensed and looks like it might be less cumbersome to use than
>>> nvpairs. =A0For 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. =A0If the decision is made to use nvpairs, I would advocate for
> emulating the Solaris API. =A0There'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. =A0Zfsd, 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. =A0Could I rewrite all
>>> that I have in C? =A0Sure, 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. =A0The 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. =A0For the polici=
es
> 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!). =A0That's too bad. =A0My hope is that, by submitting anoth=
er
> example of (dare I hope well written) C++ use in FreeBSD, that this will
> change. =A0At least review the code (when I release it in a few weeks) be=
fore
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimg33WYkuq7bTe=obTsxJdVV8Sat5THdL9E3Mu6>