Re: [PATCH v10 05/12] libs: add libxenhypfs

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Juergen Gross <jgross@suse.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Paul Durrant <paul@xen.org>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH v10 05/12] libs: add libxenhypfs
Date: Sat, 30 May 2020 16:54:57 +0100	[thread overview]
Message-ID: <8468b7ea-81ba-0512-c638-322803134576@citrix.com> (raw)
In-Reply-To: <20200519072106.26894-6-jgross@suse.com>

On 19/05/2020 08:20, Juergen Gross wrote:
> diff --git a/tools/libs/hypfs/include/xenhypfs.h b/tools/libs/hypfs/include/xenhypfs.h
> new file mode 100644
> index 0000000000..ab157edceb
> --- /dev/null
> +++ b/tools/libs/hypfs/include/xenhypfs.h
> @@ -0,0 +1,90 @@
> +/*
> + * Copyright (c) 2019 SUSE Software Solutions Germany GmbH
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef XENHYPFS_H
> +#define XENHYPFS_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <sys/types.h>
> +
> +/* Callers who don't care don't need to #include <xentoollog.h> */
> +struct xentoollog_logger;
> +
> +typedef struct xenhypfs_handle xenhypfs_handle;
> +
> +struct xenhypfs_dirent {
> +    char *name;
> +    size_t size;
> +    enum {
> +        xenhypfs_type_dir,
> +        xenhypfs_type_blob,
> +        xenhypfs_type_string,
> +        xenhypfs_type_uint,
> +        xenhypfs_type_int,
> +        xenhypfs_type_bool
> +    } type;
> +    enum {
> +        xenhypfs_enc_plain,
> +        xenhypfs_enc_gzip
> +    } encoding;
> +    bool is_writable;
> +};

I'm afraid this a blocker bug for 4.14.

enum's aren't safe in public ABI structs, even under _GNU_SOURCE.  Use
unsigned int's, and declare the enumerations earlier.

There is also 3/7 bytes of trailing padding and very little forward
extensibility.  How about an unsigned int flags, of which writeable is
the bottom bit, seeing as this is purely an informational field?

~Andrew