[libvirt] [PATCH v2 1/2] Introduce Libvirt Wireshark dissector

[Daniel P. Berrange]

On Thu, Sep 19, 2013 at 11:26:08PM +0900, Yuto KAWAMURA(kawamuray) wrote:
> diff --git a/tools/wireshark/src/moduleinfo.h b/tools/wireshark/src/moduleinfo.h
> new file mode 100644
> index 0000000..9ab642c
> --- /dev/null
> +++ b/tools/wireshark/src/moduleinfo.h
> @@ -0,0 +1,37 @@
> +/* moduleinfo.h --- Define constants about wireshark plugin module
...
> +
> +/* Included *after* config.h, in order to re-define these macros */
> +
> +#ifdef PACKAGE
> +# undef PACKAGE
> +#endif
> +
> +/* Name of package */
> +#define PACKAGE "libvirt"

Huh ?  "PACKAGE" will already be defined to 'libvirt' so why are
you redefining it.

> +
> +
> +#ifdef VERSION
> +# undef VERSION
> +#endif
> +
> +/* Version number of package */
> +#define VERSION "0.0.1"

This means the wireshark plugin will have a fixed version, even
when libvirt protocol changes in new releases. This seems bogus.
Again I think we should just use the existing defined "VERSION".

I think this whole file can just go away completely

> diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
> new file mode 100644
> index 0000000..cd3e6ce
> --- /dev/null
> +++ b/tools/wireshark/src/packet-libvirt.c
> +static gboolean
> +dissect_xdr_bytes(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf,
> +                  guint32 maxlen)
> +{
> +    goffset start;
> +    guint8 *val = NULL;
> +    guint32 length;
> +
> +    start = xdr_getpos(xdrs);
> +    if (xdr_bytes(xdrs, (char **)&val, &length, maxlen)) {
> +        proto_tree_add_bytes_format_value(tree, hf, tvb, start, xdr_getpos(xdrs) - start,
> +                                          NULL, "%s", format_xdr_bytes(val, length));
> +        /* Seems I can't call xdr_free() for this case.
> +           It will raises SEGV by referencing out of bounds argument stack */
> +        xdrs->x_op = XDR_FREE;
> +        xdr_bytes(xdrs, (char **)&val, &length, maxlen);
> +        xdrs->x_op = XDR_DECODE;

Is accessing the internals of the 'XDR' struct really portable ? I think
it would be desirable to solve the xdr_free problem rather than accessing
struct internals



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|