Wireshark · Wireshark-dev: Re: [Wireshark-dev] preliminary code submission

[Brian Oleksa <oleksab@xxxxxxxxxxxxxxxxxxxxxx>]

Chris

Thanks for all the great advice. Attached is the updated code based on 
some of your suggestions.

Per your suggestions:
I have registered the port that my dissector is using with IANA.
I have added my companies license info at the top of my dissector.
I put the handoff and register routines at the bottom.
I have defined a value_string constant for the gpsStatus.

You mentioned that the helen_sub_item assignment is not needed.
I have not removed it yet. All this displays is that it is a helen 
packet as it does start with 0xbead.
I guess your thoughts are that I do not need to display 0xbead..?? I can 
easily move this ...but I just wanted to get your thoughts on why you 
think I should not display this.

I also tried to replace FT_ABSOLUTE_TIME with FT_UNIT64 and replace the 
proto_tree_add_time_format with the add_unit_format that you provided 
but I ended up with the following error.
conversion from guint64 to guint32 possible loss of data.
But I cannot seem to locate the different value declarations.

Codes:
You mention that there is a number of Helen Packet Codes that I am not 
supporting in my dissector yet. These "extra" codes are for "add ons" or 
what we call "plugins" to the Minotaur SA application.
For now... We just want to support the core Minotaur SA extensions which 
are (GPS, Flow and Host). Once this code is submitted and a part of the 
core wireshark distribution we will submit patches to add these other 
"codes".
Right now... these extra codes are subject to change.

I did however add the range_string helen_code[]...but I am not 100% sure 
how to rip out my switch/case to replace it with your suggestion.

I have alot of your suggestions in my code...but I either commented it 
out or am not using it.

You also mentioned the following (see CHRIS MAYNARD WROTE:)...but I was 
having a bit of a problem following exactly what you are saying here.

Thanks for the help.....,
Brian

CHRIS MAYNARD WROTE:

So you're missing a lot of the codes, but even the codes that you currently do support seem to be lacking.  Take for example the GPS Extension:  First off, you don't populate the "Fields Available" field at all.  It should be added to the helen tree then sub-treed with the bits comprising it (i.e., have its own ett so it can expand independently of other trees).  It's nice to have a 1-line summary of all bitfields so you don't necessarily have to expand the tree to decipher which bits are set.  The Boolean bits themselves are usually populated from msb to lsb, although the fields indicated must be populated according to the "spec".  Unfortunately, the Helen "spec" is ambiguous in this regard.  Here's an example of what the tree could look like as I describe (I assume the fields are added according to lsb to msb based on your implementation, but I indicate which fields are present from msb to lsb, as this yields a left-to-right expansion of the tree, which is what you normally see.):

Not expanded, you would see:

+ Fields Available: 0x61 (speed, bearing, status)

And when expanded, you would see:

- Fields Available: 0x61 (speed, bearing, status)
 0....... = Number of Satellites: Not Present
 .1...... = Speed: Present
 ..1..... = Bearing: Present
 ...0.... = Altitude: Not Present
 ....0... = Latitude: Not Present
 .....0.. = Longitude: Not Present
 ......0. = Time: Not Present
 .......1 = Status: Present








Maynard, Chris wrote:
> Hi Brian,
>
> I found some (the?) documentation on Helen here: https://www.darkcornersoftware.com/confluence/display/open/Packet+Structure.  I have no idea if this is the definitive Helen specification or not, but based on the documentation, here is some additional feedback:
>
> Header:
> This 1st assignment of helen_sub_item is not needed:
>         helen_sub_item = proto_tree_add_item(helen_tree, hf_helen_magic, tvb, offset, 2, FALSE);
>
> The System Tx Time: is 8 bytes and indicates ms since the Epoch.  Your hf_helen_txTime field is declared as an FT_ABSOLUTE_TIME, but that implies that you have an 8 byte field whose 1st 4 bytes represent seconds and last 4 represent nanoseconds, but that isn't the case.  This field is simply an FT_INT64 (or FT_UINT64 if you don't care to represent ms before the Epoch).  The Helen "spec" is ambiguous as to whether it's signed or unsigned, but I suspect signed since I saw some Java references in there.
>
> With that in mind, there's no reason at all to bother with nanoseconds as you wouldn't be adding an FT_ABSOLUTE_TIME to the tree, you'd be adding the FT_INT64.  All this just means changing this:
>         proto_tree_add_time_format(helen_tree, hf_helen_txTime, tvb, offset, 8, &t,
>             "Date: %s %2d, %d %02d:%02d:%02d UTC",mon_names[tmp->tm_mon],tmp->tm_mday,
> 			tmp->tm_year + 1900,tmp->tm_hour,tmp->tm_min,tmp->tm_sec,(long)t.nsecs);
> to this:
>         proto_tree_add_uint_format(helen_tree, hf_helen_txTime, tvb, offset, 8, msecs_since_the_epoch,
>             "Date: %s %2d, %d %02d:%02d:%02d UTC",mon_names[tmp->tm_mon],tmp->tm_mday,
> 			tmp->tm_year + 1900,tmp->tm_hour,tmp->tm_min,tmp->tm_sec, msecs_since_the_epoch%1000);
>
> And of course using gmtime() means you have an inherent year 2038 problem: http://en.wikipedia.org/wiki/Year_2038_problem, but since this field is in ms, it won't be wrong for almost another full second after everyone else ... so you've got that going for you! :)
>
> Codes:
> There seem to be number of Helen Packet Codes defined that your dissector doesn't yet support.  It's already been mentioned below about using value_string, but I suggest you use a range_string for the codes.  For example: 
>
> static const range_string helen_code[] = {
>     {0x8000, 0xfffe, "Experimental"},
>     {0xffff, 0xffff, "Encryption Extension"},
>     {0, 0, "Tail"},
>     {1, 1, "GPS Extension"},
>     {2, 2, "Flow Extension"},
>     {3, 3, "Host Extension"},
>     {4, 4, "File Extension"},
>     {5, 999, "Reserved"},
>     {1000, 1000, "Minotaur SA Extension"},
>     ...
>     {16000, 32767, "Unassigned"},
>     {0, 0, NULL}
> };
>
> Read more about range_string in doc/README.developer, paying close attention to RVALS() and BASE_RANGE_STRING.  Then, when you add the code to the tree, you simply do something like this for each code:
>
> proto_tree_add_item(helen_ext_tree, hf_helen_code, tvb, offset, 2, FALSE);
>
> You should probably use at least one new ett_, such as ett_helen_ext for the extensions, rather than using ett_helen.  Alternatively, you could have a unique ett for each extension if you want.
>
> So you're missing a lot of the codes, but even the codes that you currently do support seem to be lacking.  Take for example the GPS Extension:  First off, you don't populate the "Fields Available" field at all.  It should be added to the helen tree then sub-treed with the bits comprising it (i.e., have its own ett so it can expand independently of other trees).  It's nice to have a 1-line summary of all bitfields so you don't necessarily have to expand the tree to decipher which bits are set.  The Boolean bits themselves are usually populated from msb to lsb, although the fields indicated must be populated according to the "spec".  Unfortunately, the Helen "spec" is ambiguous in this regard.  Here's an example of what the tree could look like as I describe (I assume the fields are added according to lsb to msb based on your implementation, but I indicate which fields are present from msb to lsb, as this yields a left-to-right expansion of the tree, which is what you normally see.):
>
> Not expanded, you would see:
>
> + Fields Available: 0x61 (speed, bearing, status)
>
> And when expanded, you would see:
>
> - Fields Available: 0x61 (speed, bearing, status)
>   0....... = Number of Satellites: Not Present
>   .1...... = Speed: Present
>   ..1..... = Bearing: Present
>   ...0.... = Altitude: Not Present
>   ....0... = Latitude: Not Present
>   .....0.. = Longitude: Not Present
>   ......0. = Time: Not Present
>   .......1 = Status: Present
>
> Have a look at epan/dissectors/packet-ip.c, which contains some examples of this type of bit expansion in the "Differentiated Services Field" and "Flags" fields.
>
> As for the helen gps extension fields themselves, Mike and Gerasimos already gave you good advice on reducing the code to add the status code ... although it can be reduced even further:
>
>     if (fieldsAvail & GPS_EXT_STATUS_MASK)
>         proto_tree_add_item(helen_sub_tree, hf_helen_gpsstatus, tvb, offset++, 1, FALSE);
>
> Hopefully this gives you some helpful pointers.
>
> A few parting tips: Be sure to read doc/README.developer, try to use proto_tree_add_item() as much as possible, avoid proto_tree_add_text() unless you really have no idea what the data represents, if you can't use proto_tree_add_item() or don't want to for whatever reason, keep in mind what the type of data the payload actually represents and use the appropriate proto_tree_add_xyz() function to do it.  For example, don't read an integer, format it into a string and then use proto_tree_add_string_format().  The data is not a string, it's an integer, so you would use proto_tree_add_uint_format(), assuming it's unsigned.  Lastly, be sure to take advantage of the many existing dissectors in epan/dissectors/packet-*.c as guides to help you further improve your dissector.  Look at common ones (ip, udp, ...) or ones whose protocols you have some familiarity with so you understand the data presented, then go look at how the author accomplished it.
>
> Good luck.
> - Chris
>
> -----Original Message-----
> From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Maynard, Chris
> Sent: Thursday, February 04, 2010 10:37 AM
> To: Developer support list for Wireshark
> Subject: Re: [Wireshark-dev] preliminary code submission
>
> Some other observations/feedback:
>
> You should look again at the doc/README.developer skeleton code and follow the common structure there.  For example, you need to add a GPL statement, you should move the handoff and register routines to the bottom, etc.
>
> Run tools/check*.pl on your dissector.
> Fuzztest your dissector if you haven't already done so.
>
> I would recommend breaking up your for() loop into smaller functions and add some handling for an unknown code.  For example:
> for (;;) {
> 	...
> 	if (code == 0)
> 		break;
> 	else if (code == 1)
> 		offset += dissect_gps(...);
> 	else if (code == 2)
> 		offset += dissect_flow(...);
> 	else if (code == 3)
> 		offset += dissect_host(...);
> 	else
> 		offset += dissect_unknown(...);
> 	...
> }
>
> Or replace the if()/else if()/else structure with a switch() and use an appropriate condition to exit your for() loop.  Or you could use a vector table, i.e., 
>
> #define MAX_CODES	4
> static guint32 (*dissect_code[MAX_CODES])(...) = {
> 	dissect_null, dissect_gps, dissect_flow, dissect_host
> };
> 	if (code < MAX_CODES)
> 		offset += dissect_code[code](...);
> 	else
> 		offset += dissect_unknown(...);
>
> Port 7636 is not registered with IANA.  http://www.iana.org/assignments/port-numbers 
> Is that a port you picked?  You should either register it or add a preference to make the port configurable or choose a port past the registered port range. 
>
> Is this protocol documented anywhere?  You should reference the RFC(s) or whatever it is that describes it.
>
> - Chris
>
> -----Original Message-----
> From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Gerasimos Dimitriadis
> Sent: Thursday, February 04, 2010 4:40 AM
> To: Developer support list for Wireshark
> Subject: Re: [Wireshark-dev] preliminary code submission
>
> Hi Brian,
>
> there is no need to add a text string as blurb, if it is the same as
> the title of the field, so for example the ipv4 one would become:
> 	    { &hf_helen_ipv4,
> 			{ "IPv4", "helen.ipv4address", FT_IPv4, BASE_NONE, NULL, 0x0,
> 	            NULL, HFILL}},
>
> Also, continuing from Michael's comments there is no need to read the
> value of status and use proto_tree_add_uint, since you are not using
> status later on. I think that it would be best to change it to:
>
> void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
> {
>        [...]
>        /* Status: */
>        if ((fieldsAvail & 1) != 0) {
>                proto_tree_add_item(helen_sub_tree, hf_helen_gpsstatus,
> tvb, offset, 1, FALSE);
>                offset += 1;
>        }
>        [...]
> }
>
> Also at dissect_helen, I propose to deal with the packet code in the
> same way as the gpsstatus. Add an entry for it in the hf[] array,
> define an array of value_string with the code names (i.e. End of
> Packet, GPS Extension...) and use proto_tree_add_item to add it to the
> tree. Apart from all other advantages, you don't need to explicitly
> deal with the unknown code strings.
>
> Best regards,
>
> Gerasimos
>
> 2010/2/4 Speck Michael EHWG AVL/GAE <Michael.Speck@xxxxxxx>:
>   
> > Hi Brian,
> >
> > just had a look on your latest preview.
> > There is one more thing you can do to reduce the code size even further:
> > as Jakub has already suggested earlier, it is a good idea to use
> > value_string struct to map values with strings. In your case it is
> > especially true for the GPS status.
> >
> > There are only a few changes necessary to your code. First define a
> > value_string typed constant, then slightly change the status field
> > definition in function proto_register_helen() and finally shorten the
> > status field dissection in function dissect_helen(). Below is shown how
> > these code sections could look like afterwards.
> >
> >
> > static const value_string helen_gps_status[] = {
> >        { 0, "Good" },
> >        { 1, "No Fix" },
> >        { 2, "Bad GPS Read" },
> >        { 0, NULL }
> > };
> >
> >
> > void proto_register_helen(void) {
> > [...]
> >        { &hf_helen_gpsstatus,
> >                { "GPS Status", "helen.gpsStatus", FT_UINT8, BASE_DEC,
> > VALS(helen_gps_status), 0x00,
> >                "GPS Status", HFILL}},
> > [...]
> > }
> >
> >
> > void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
> > {
> >        [...]
> >        /* Status: */
> >        if ((fieldsAvail & 1) != 0) {
> >                guint8 status;
> >                status = tvb_get_guint8(tvb,offset);
> >                proto_tree_add_uint(helen_sub_tree, hf_helen_gpsstatus,
> > tvb, offset, 1, status);
> >                offset += 1;
> >        }
> >        [...]
> > }
> >
> >
> >
> > Cheers
> > Mike
> >
> >
> >
> >
> > -----Original Message-----
> > From: wireshark-dev-bounces@xxxxxxxxxxxxx
> > [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Brian Oleksa
> > Sent: Mittwoch, 3. Februar 2010 21:21
> > To: Developer support list for Wireshark
> > Subject: Re: [Wireshark-dev] preliminary code submission
> >
> > Jakub
> >
> > Yes...you are right. I am not using alot of those variables. I was
> > mislead by what I was doing before I started to use the built in
> > routines. :-)
> >
> > My code base keeps getting smaller and smaller. I do understand that
> > code quality / readability is a key factor in this industry. I still
> > need to fix my IDE to get the correct formatting.
> >
> > Perhaps once last quick look..?? Attached is the updated file.
> >
> > Thanks again for the great feedback..!!
> >
> > Brian
> >
> >
> >
> >
> >
> > Jakub Zawadzki wrote:
> >     
> > > Hi,
> > >
> > > On Wed, Feb 03, 2010 at 01:05:32PM -0500, Brian Oleksa wrote:
> > >
> > >       
> > > > Jakub
> > > >
> > > > Thanks for this feedback. It is always good to have an extra set of
> > > > eyes :-)
> > > >
> > > >         
> > > You missunderstood my comment about check_col()
> > >
> > > instead of:
> > >     if (check_col(pinfo->cinfo, COL_PROTOCOL))
> > >         col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
> > >       
> > >     if (check_col(pinfo->cinfo, COL_INFO))
> > >         col_clear(pinfo->cinfo, COL_INFO);
> > >
> > > Just write:
> > >       col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
> > >       
> > >         col_clear(pinfo->cinfo, COL_INFO);
> > >
> > >
> > >       
> > > > gfloat latitude;
> > > > latitude = tvb_get_ntohieee_float(tvb,offset);
> > > >
> > > > Why do you think this variable is not being used..??
> > > >
> > > >         
> > > Well because it's not used :) What is used for?
> > >
> > > And one more thing...
> > >
> > > You declare: ett_helen_ipv6, ett_helen_nos, ...
> > > I believe you need only ett_helen from ett_*
> > >
> > > Cheers.
> > >
> > >       
> CONFIDENTIALITY NOTICE: The contents of this email are confidential
> and for the exclusive use of the intended recipient. If you receive this
> email in error, please delete it from your system immediately and 
> notify us either by email, telephone or fax. You should not copy,
> forward, or otherwise disclose the content of the email.
>
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>   
/* ***** BEGIN LICENSE BLOCK *****
 * Version: DCSPL 1.1
 *
 * The contents of this file are subject to the Dark Corner Software Public
 * License Version 1.1 (the "License"); you may not use this file except in
 * compliance with the License.  You may obtain a copy of the License at
 * http://www.darkcornersoftware.com/DCSPL/
 *
 * Software distributed under the License is distributed on an "AS IS"
 * basis, WITHOUT WARRANTY OF ANY KIND, either express or implied.  See the
 * License for the specific language governing rights and limitations
 * under the License.
 *
 * The Initial Developer of the Original Code is Dark Corner Software LLC.
 * Portions created by Dark Corner Software LLC are Copyright (C) 2007 Dark
 * Corner Software LLC.  All Rights Reserved.
 *
 * Contributor(s):
 *   None
 *
 */

#ifdef HAVE_CONFIG_H
#include "config.h"
#endif

#include <stdio.h>
#include <glib.h>
#include <epan/packet.h>
#include <time.h>
#include <string.h>

#define PROTO_TAG_HELEN    "HELEN"

static int proto_helen = -1;

static dissector_handle_t helen_handle;

void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree);

/*The following port is registered with IANA*/
static int helen_port = 5136;

static const char *mon_names[12] = {"Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"};

static const value_string helen_gps_status[] = {
	{ 0, "Good" },
	{ 1, "No Fix" },
	{ 2, "Bad GPS Read" },
	{ 0, NULL }
};

static const range_string helen_code[] = {
    {0x8000, 0xfffe, "Experimental"},
    {0xffff, 0xffff, "Encryption Extension"},
    {0, 0, "Tail"},
    {1, 1, "GPS Extension"},
    {2, 2, "Flow Extension"},
    {3, 3, "Host Extension"},
    {4, 4, "File Extension"},
    {5, 999, "Reserved"},
    {1000, 1000, "Minotaur SA Extension"},
    {16000, 32767, "Unassigned"},
    {0, 0, NULL}
};

static gint hf_helen_magic = -1;
static gint hf_helen_checksum = -1;
static gint hf_helen_txTime = -1;

static gint hf_helen = -1;
static gint hf_helen_time = -1;
static gint hf_helen_ipv4 = -1;
static gint hf_helen_ipv6 = -1;
static gint hf_helen_nos = -1;
static gint hf_helen_flowname = -1;
static gint hf_helen_longitude = -1;
static gint hf_helen_latitude = -1;
static gint hf_helen_altitude = -1;
static gint hf_helen_bearing = -1;
static gint hf_helen_speed = -1;
static gint hf_helen_sequence_num = -1;
static gint hf_helen_gpsstatus = -1;
static gint hf_helen_source = -1;
static gint hf_helen_code = -1;

static gint ett_helen = -1;

void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree) {

    proto_item *helen_item = NULL;
    proto_item *helen_sub_item = NULL;
    proto_tree *helen_tree = NULL;
    proto_tree *helen_header_tree = NULL;

	col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
    col_clear(pinfo->cinfo, COL_INFO);

    if (tree) {
        guint32 offset = 0;
        guint32 orig_offset = 18;
        nstime_t t;
		guint64 msecs_since_the_epoch;
		struct tm *tmp;
        helen_item = proto_tree_add_item(tree, proto_helen, tvb, 0, -1, FALSE);
        helen_tree = proto_item_add_subtree(helen_item, ett_helen);
        helen_header_tree = proto_item_add_subtree(helen_item, ett_helen);
        helen_sub_item = proto_tree_add_item(helen_tree, hf_helen_magic, tvb, offset, 2, FALSE);
        offset += 2;
		helen_sub_item = proto_tree_add_item(helen_tree, hf_helen_checksum, tvb, offset, 8, FALSE);
		offset += 8;

		msecs_since_the_epoch = tvb_get_ntoh64(tvb, offset);
		t.secs = msecs_since_the_epoch/1000;
		t.nsecs = (msecs_since_the_epoch%1000)*1000000;	/* milliseconds to nanoseconds */
		tmp = gmtime(&t.secs);

        proto_tree_add_time_format(helen_tree, hf_helen_txTime, tvb, offset, 8, &t,
            "Date: %s %2d, %d %02d:%02d:%02d UTC",mon_names[tmp->tm_mon],tmp->tm_mday,
			tmp->tm_year + 1900,tmp->tm_hour,tmp->tm_min,tmp->tm_sec,(long)t.nsecs);

		//proto_tree_add_uint_format(helen_tree, hf_helen_txTime, tvb, offset, 8, msecs_since_the_epoch,
		//	"Date: %s %2d, %d %02d:%02d:%02d UTC",mon_names[tmp->tm_mon],tmp->tm_mday,
		//	tmp->tm_year + 1900,tmp->tm_hour,tmp->tm_min,tmp->tm_sec, msecs_since_the_epoch%1000);

        helen_header_tree = proto_item_add_subtree(helen_sub_item, ett_helen);
        {
            #define MAX_BUFFER 100
			char *buf = (char*)ep_alloc(MAX_BUFFER);
            char * packet_name = "";
            proto_tree *helen_sub_tree = NULL;
            offset = 18;

            for (;;) {
                guint16 code;
                guint16 numBytes = 0;
                guint unknownPacket = 0;
                guint codeOffset;
                offset = orig_offset;
		    	code = tvb_get_ntohs(tvb, offset);
                codeOffset = offset;
                offset += 2;
                numBytes = tvb_get_ntohs(tvb, offset);
                offset += 2;

                switch (code) {
                    case 0: packet_name = "End of Packet";
                        break;
                    case 1: packet_name = "GPS Extension";
                        break;
                    case 2: packet_name = "Flow Extension";
                        break;
                    case 3: packet_name = "Host Extension";
                        break;
                    default: packet_name = "Unknown code";
                        unknownPacket = 1;
                        break;
                }

				//proto_tree_add_item(helen_ext_tree, hf_helen_code, tvb, offset, 2, FALSE);

                g_snprintf(buf, MAX_BUFFER, "%s", packet_name);

                if (unknownPacket) {
                    g_snprintf(buf, MAX_BUFFER, "Unknown packet: %d", code);
                }

                helen_item = proto_tree_add_text(tree, tvb, codeOffset, 2, "%s", buf);
                helen_sub_tree = proto_item_add_subtree(helen_item, ett_helen);

                if (code == 0) {
                    break;
                }

                /* GPS: */
                if (code == 1) {
                    guint8 fieldsAvail;
                    fieldsAvail = tvb_get_guint8(tvb, offset);
                    offset += 1;

                    /* Status: */
                    if ((fieldsAvail & 1) != 0) {
                 		proto_tree_add_uint(helen_sub_tree, hf_helen_gpsstatus ,tvb , offset, 1, FALSE);
                        offset += 1;
                    }

                    /* Time: */
                    if ((fieldsAvail & 2) != 0) {
						nstime_t t;
						guint64 msecs_since_the_epoch;
						struct tm *tmp;
					    msecs_since_the_epoch = tvb_get_ntoh64(tvb, offset);
						t.secs = msecs_since_the_epoch/1000;
						t.nsecs = (msecs_since_the_epoch%1000)*1000000;	/* milliseconds to nanoseconds */
						tmp = gmtime(&t.secs);
                        proto_tree_add_time_format(helen_sub_tree, hf_helen_time, tvb, offset, 8, &t,
                                "Date: %s %2d, %d %02d:%02d:%02d UTC",mon_names[tmp->tm_mon],tmp->tm_mday,
								tmp->tm_year + 1900,tmp->tm_hour,tmp->tm_min,tmp->tm_sec,(long)t.nsecs);
                        offset += 8;
                    }

                    /* Longitude: */
                    if ((fieldsAvail & 4) != 0) {
                        proto_tree_add_item(helen_sub_tree, hf_helen_longitude, tvb, offset, 4, FALSE);
                        offset += 4;
                    }

                    /* Latitude: */
                    if ((fieldsAvail & 8) != 0) {
                        proto_tree_add_item(helen_sub_tree, hf_helen_latitude, tvb, offset, 4, FALSE);
                        offset += 4;
                    }

                    /* Altitude: */
                    if ((fieldsAvail & 16) != 0) {
                        proto_tree_add_item(helen_sub_tree, hf_helen_altitude, tvb, offset, 4, FALSE);
                        offset += 4;
                    }

                    /* Bearing: */
                    if ((fieldsAvail & 32) != 0) {
                        proto_tree_add_item(helen_sub_tree, hf_helen_bearing, tvb, offset, 4, FALSE);
                        offset += 4;
                    }

                    /* Speed: */
                    if ((fieldsAvail & 64) != 0) {
                        gfloat speed;
                        speed = tvb_get_ntohieee_float(tvb,offset);;
                        if (speed != 0.0) {
							proto_tree_add_item(helen_sub_tree, hf_helen_speed, tvb, offset, 4, FALSE);
                        }
                        offset += 4;
                    }

                    /* Number of Satellites: */
                    if ((fieldsAvail & 128) != 0) {
                        proto_tree_add_item(helen_sub_tree, hf_helen_nos, tvb, offset, 1, FALSE);
                        offset += 1;
                    }
                }

                /* FLOW: */
                if (code == 2) {
                    proto_tree_add_item(helen_sub_tree, hf_helen_flowname, tvb, offset, 8, FALSE);
                    offset += 8;

                    /* Sequence number: */
        			proto_tree_add_item(helen_sub_tree, hf_helen_sequence_num, tvb, offset, 4, FALSE);
                    offset += 4;

                    if (numBytes == 16) {
					   /* Source: */
					   proto_tree_add_item(helen_sub_tree, hf_helen_source, tvb, offset, 4, FALSE);
					   offset += 4;
                    }
                }

                /* HOST: */
                if (code == 3) {
                    /* Size: */
                    guint8 size;
                    size = tvb_get_guint8(tvb, offset);
                    offset += 1;

                    if (size == 4) {
                        proto_tree_add_item(helen_sub_tree, hf_helen_ipv4, tvb, offset, 4, FALSE);
                        offset += 4;
                    } else
                    {
						proto_tree_add_item(helen_sub_tree, hf_helen_ipv6, tvb, offset, 16, FALSE);
						offset += 16;
					}
			    }
                orig_offset += numBytes + 4;
            }
        }
    }
}

void proto_reg_handoff_helen(void) {
    static gboolean initialized = FALSE;

        if (!initialized) {
            helen_handle = create_dissector_handle(dissect_helen, proto_helen);
            dissector_add("udp.port", helen_port, helen_handle);
        }
    initialized = TRUE;
}

void proto_register_helen(void) {
    static hf_register_info hf[] = {
        { &hf_helen,
            { "Data", "helen.data", FT_NONE, BASE_NONE, NULL, 0x0,
                "HELEN PDU", HFILL}},
        { &hf_helen_magic,
            { "Magic Number", "helen.magicNumber", FT_UINT8, BASE_HEX, NULL, 0x0,
                NULL, HFILL}},
        { &hf_helen_checksum,
           { "Checksum", "helen.checksum", FT_UINT64, BASE_DEC, NULL, 0x0,
                NULL, HFILL}},
        { &hf_helen_txTime,
            { "System Tx Time", "helen.SystemTxTime", FT_ABSOLUTE_TIME, BASE_NONE, NULL, 0x0,
                NULL, HFILL}},
        { &hf_helen_time,
	        { "Time", "helen.time", FT_ABSOLUTE_TIME, BASE_NONE, NULL, 0x0,
	            NULL, HFILL}},
	    { &hf_helen_ipv4,
			{ "IPv4", "helen.ipv4address", FT_IPv4, BASE_NONE, NULL, 0x0,
	            NULL, HFILL}},
	    { &hf_helen_ipv6,
			{ "IPv6", "helen.ipv6address", FT_IPv6, BASE_NONE, NULL, 0x0,
	            NULL, HFILL}},
        { &hf_helen_sequence_num,
			{ "Sequence Number", "helen.sequenceNumber", FT_UINT32, BASE_DEC, NULL, 0x0,
                NULL, HFILL}},
        { &hf_helen_source,
			{ "Source", "helen.source", FT_UINT32, BASE_DEC, NULL, 0x0,
                NULL, HFILL}},
        { &hf_helen_nos,
			{ "Number of Satellites", "helen.numberOfSatellites", FT_UINT8, BASE_DEC, NULL, 0x0,
			NULL, HFILL}},
		{ &hf_helen_flowname,
			{ "Flowname", "helen.flowname", FT_STRING, BASE_NONE, NULL, 0x0,
			NULL, HFILL}},
		{ &hf_helen_gpsstatus,
			{ "GPS Status", "helen.gpsStatus", FT_UINT8, BASE_DEC, VALS(helen_gps_status), 0x0,
			NULL, HFILL}},
		{ &hf_helen_code,
			{ "Packet COde", "helen.packetCode", FT_UINT8, BASE_DEC, VALS(helen_code), 0x0,
			NULL, HFILL}},
		{ &hf_helen_longitude,
			{ "Longitude", "helen.longitude", FT_FLOAT, BASE_DEC, NULL, 0x0,
			NULL, HFILL}},
		{ &hf_helen_latitude,
			{ "Latitude", "helen.latitude", FT_FLOAT, BASE_DEC, NULL, 0x0,
			NULL, HFILL}},
		{ &hf_helen_altitude,
			{ "Altitude", "helen.altitude", FT_FLOAT, BASE_DEC, NULL, 0x0,
			NULL, HFILL}},
		{ &hf_helen_bearing,
			{ "Bearing", "helen.bearing", FT_FLOAT, BASE_DEC, NULL, 0x0,
			NULL, HFILL}},
		{ &hf_helen_speed,
			{ "Speed", "helen.speed", FT_FLOAT, BASE_DEC, NULL, 0x0,
			NULL, HFILL}},
    };

    static gint * ett[] = {&ett_helen};

    proto_helen = proto_register_protocol("HELEN", "HELEN", "helen");
    proto_register_field_array(proto_helen, hf, array_length(hf));
    proto_register_subtree_array(ett, array_length(ett));
    register_dissector("helen", dissect_helen, proto_helen);
}