> +int mlx4_query_diag_counters(struct mlx4_dev *dev, int array_length,
> + int in_modifier, unsigned int in_offset[],
> + u32 counter_out[])
> +{
> + struct mlx4_cmd_mailbox *mailbox;
> + u32 *outbox;
> + u32 op_modifer = (u32)in_modifier;
This coding style looks strange to me... you have an int parameter
in_modifier that is not used for anything except to assign it to a u32
op_modifer [sic] variable with a (u32) cast that doesn't do anything.
Why not just have op_modifier be the parameter in the first place?
Also the array_length stuff looks kind of funny since you only ever pass
in a value of 1... why not just pass in int offset and u32 *counter?
> + /* clear counters file, can't read it */
> + if(offset < 0)
> + return sprintf(buf,"This file is write only\n");
Why not just set the permissions on the file so it can't be opened for
reading? This just looks like a recipe for making userspace code go
crazy on unexpected input.
Also watch out for the space in "if ("
And if I'm understanding correctly, you use a magic offset of -1 for the
clear_diag attribute that makes mlx4_query_diag_counters() read before
the beginning of the output mailbox.
> +err_diag:
> + ib_unregister_device(&ibdev->ib_dev);
> +
> err_reg:
> ib_unregister_device(&ibdev->ib_dev);
This doesn't look like a good idea.
- R.
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
