Re: [Qemu-devel] [PATCH 4/6] libqblock internal used functions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

On 09/03/2012 03:18 AM, Wenchao Xia wrote:
>   This patch contains internal helper codes.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c                      |    2 +-
>  block.h                      |    1 +
>  libqblock/libqblock-helper.c |   92 ++++++++++++++++++++++++++++++++++++++++++
>  libqblock/libqblock-helper.h |   57 ++++++++++++++++++++++++++
>  4 files changed, 151 insertions(+), 1 deletions(-)
>  create mode 100644 libqblock/libqblock-helper.c
>  create mode 100644 libqblock/libqblock-helper.h
> 

> +++ b/libqblock/libqblock-helper.c
> @@ -0,0 +1,92 @@
> +#include "libqblock-helper.h"

No copyright.  Shame.  I'll quit pointing it out for the rest of the series.

> +++ b/libqblock/libqblock-helper.h

> +
> +/* this file contains helper function used internally. */
> +#define SECTOR_SIZE (512)

Hard-coding this feels wrong, in this day and age of disks with 4096
sectors.  Why isn't this a per-image property?

> +#define SECTOR_SIZE_MASK (0x01ff)
> +#define SECTOR_SIZE_BITS_NUM (9)

and these computed from a dynamic per-image sector size?

> +#define FUNC_FREE free
> +#define FUNC_MALLOC malloc
> +#define FUNC_CALLOC calloc

Is libqblock-helper.h intended to be installed and included in client
applications, or is it internal only?  If clients can ever use this
header, then you need to avoid namespace violations, by using naming
such as QB_SECTOR_SIZE, QB_FUNC_FREE, and so forth.

> +
> +const char *fmt2str(enum QBlockFormat fmt);
> +enum QBlockFormat str2fmt(const char *fmt);

Even if this header is not intended for clients, you STILL need to avoid
namespace pollution, by either ensuring that your library marks
visibility annotations to avoid exporting symbols outside of the public
namespace, or by renaming even your internal functions to comply with
the namespace.  In other words, stomping on the name 'fmt2str' and
'str2fmt' as an external function name is just too likely to clash with
an arbitrary client linking against your library.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]