On 25.04.2016 23:24, Cole Robinson wrote:
> On 04/25/2016 10:38 AM, Nikolay Shirokovskiy wrote:
>> Daemon config parameter switch between reading host uuid
>> either from smbios or machine-id:
>>
>> host_uuid_source = "smbios|machine-id"
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>> ---
>> daemon/libvirtd-config.c | 2 ++
>> daemon/libvirtd-config.h | 1 +
>> daemon/libvirtd.c | 38 +++++++++++++++++++++++++++++++++++---
>> daemon/libvirtd.conf | 14 ++++++++++----
>> src/libvirt_private.syms | 1 +
>> src/util/viruuid.c | 31 ++++++++++++++++++++++---------
>> src/util/viruuid.h | 3 +++
>> 7 files changed, 74 insertions(+), 16 deletions(-)
>>
>> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
>> index 7a448f9..45280e9 100644
>> --- a/daemon/libvirtd-config.c
>> +++ b/daemon/libvirtd-config.c
>> @@ -374,6 +374,7 @@ daemonConfigFree(struct daemonConfig *data)
>> VIR_FREE(data->crl_file);
>>
>> VIR_FREE(data->host_uuid);
>> + VIR_FREE(data->host_uuid_source);
>> VIR_FREE(data->log_filters);
>> VIR_FREE(data->log_outputs);
>>
>> @@ -463,6 +464,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
>> GET_CONF_UINT(conf, filename, audit_logging);
>>
>> GET_CONF_STR(conf, filename, host_uuid);
>> + GET_CONF_STR(conf, filename, host_uuid_source);
>>
>> GET_CONF_UINT(conf, filename, log_level);
>> GET_CONF_STR(conf, filename, log_filters);
>> diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
>> index 3e1971d..672e9ad 100644
>> --- a/daemon/libvirtd-config.h
>> +++ b/daemon/libvirtd-config.h
>> @@ -28,6 +28,7 @@
>>
>> struct daemonConfig {
>> char *host_uuid;
>> + char *host_uuid_source;
>>
>> int listen_tls;
>> int listen_tcp;
>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>> index 7ec02ad..ca1ec7a 100644
>> --- a/daemon/libvirtd.c
>> +++ b/daemon/libvirtd.c
>> @@ -1076,6 +1076,39 @@ static int migrateProfile(void)
>> return ret;
>> }
>>
>> +static int
>> +daemonSetupHostUUID(const struct daemonConfig *config)
>> +{
>> + static const char *machine_id = "/etc/machine-id";
>> + char buf[VIR_UUID_STRING_BUFLEN];
>> + const char *uuid;
>> +
>> + if (config->host_uuid) {
>> + uuid = config->host_uuid;
>> + } else if (!config->host_uuid_source ||
>> + STREQ(config->host_uuid_source, "smbios")) {
>> + /* set host uuid in lazy manner*/
>
> How about
>
> /* smbios UUID is fetched on demand in virGetHostUUID */
>
>> + return 0;
>> + } else if (STREQ(config->host_uuid_source, "machine-id")) {
>> + if (virUUIDRead(machine_id, buf, sizeof(buf)) < 0) {
>> + VIR_ERROR(_("Can't read %s"), machine_id);
>> + return -1;
>> + }
>> +
>> + uuid = buf;
>> + } else {
>> + VIR_ERROR(_("invalid UUID source: %s"), config->host_uuid_source);
>> + return -1;
>> + }
>> +
>> + if (virSetHostUUIDStr(uuid)) {
>> + VIR_ERROR(_("invalid host UUID: %s"), uuid);
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /* Print command-line usage. */
>> static void
>> daemonUsage(const char *argv0, bool privileged)
>> @@ -1295,9 +1328,8 @@ int main(int argc, char **argv) {
>> exit(EXIT_FAILURE);
>> }
>>
>> - if (config->host_uuid &&
>> - virSetHostUUIDStr(config->host_uuid) < 0) {
>> - VIR_ERROR(_("invalid host UUID: %s"), config->host_uuid);
>> + if (daemonSetupHostUUID(config) < 0) {
>> + VIR_ERROR(_("Can't setup host uuid"));
>> exit(EXIT_FAILURE);
>> }
>>
>> diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
>> index d2c439c..eaae680 100644
>> --- a/daemon/libvirtd.conf
>> +++ b/daemon/libvirtd.conf
>> @@ -410,10 +410,15 @@
>>
>> ###################################################################
>> # UUID of the host:
>> -# Provide the UUID of the host here in case the command
>> -# 'dmidecode -s system-uuid' does not provide a valid uuid. In case
>> -# 'dmidecode' does not provide a valid UUID and none is provided here, a
>> -# temporary UUID will be generated.
>> +# host uuid is read from one of host uuid sources specified
>> +# in host_uuid_source. Possible values are 'smbios' and 'machine-id'.
>> +# First option will set host uuid from 'dmidecode -s system-uuid',
>> +# while second will read it from /etc/machine-id. uuid source
>> +# defaults to smbios. Other option is to specify host uuid
>> +# right here in host_uuid option, in this case host_uuid_source
>> +# will not be taken into account. In case uuid value in
>> +# smbios is invalid temporary host uuid will be generated.
>> +#
>
> How about:
>
> Host UUID is read from one of the sources specified in host_uuid_source.
>
> - 'smbios': fetch the UUID from 'dmidecode -s system-uuid'
> - 'machine-id': fetch the UUID from /etc/machine-id
>
> The host_uuid_source default is 'smbios'. If 'dmidecode' does not provide
> a valid UUID and none is provided here, a temporary UUID will be generated.
>
> A UUID can be manually specified with host_uuid, in which case
> host_uuid_source is not used.
>
>
>> # Keep the format of the example UUID below. UUID must not have all digits
>> # be the same.
>>
>> @@ -421,6 +426,7 @@
>> # it with the output of the 'uuidgen' command and then
>> # uncomment this entry
>> #host_uuid = "00000000-0000-0000-0000-000000000000"
>> +#host_uuid_source = "smbios"
>>
>> ###################################################################
>> # Keepalive protocol:
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 2b55369..e5823b2 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2512,6 +2512,7 @@ virUUIDFormat;
>> virUUIDGenerate;
>> virUUIDIsValid;
>> virUUIDParse;
>> +virUUIDRead;
>>
>>
>> # util/virxml.h
>> diff --git a/src/util/viruuid.c b/src/util/viruuid.c
>> index 16e57db..721eac0 100644
>> --- a/src/util/viruuid.c
>> +++ b/src/util/viruuid.c
>> @@ -231,21 +231,34 @@ getDMISystemUUID(char *uuid, int len)
>> };
>>
>> while (paths[i]) {
>> - int fd = open(paths[i], O_RDONLY);
>> - if (fd >= 0) {
>> - if (saferead(fd, uuid, len - 1) == len - 1) {
>> - uuid[len - 1] = '\0';
>> - VIR_FORCE_CLOSE(fd);
>> - return 0;
>> - }
>> - VIR_FORCE_CLOSE(fd);
>> - }
>> + if (virUUIDRead(paths[i], uuid, len) == 0)
>> + return 0;
>> i++;
>> }
>>
>> return -1;
>> }
>>
>> +int
>> +virUUIDRead(const char *file, char *uuid, int len)
>> +{
>> + int fd;
>> + ssize_t sz;
>> +
>> + fd = open(file, O_RDONLY);
>> + if (fd < 0)
>> + return -1;
>> +
>> + if ((sz = saferead(fd, uuid, len - 1)) < 0)
>> + goto cleanup;
>> +
>> + uuid[sz] = '\0';
>> +
>> + cleanup:
>> + VIR_FORCE_CLOSE(fd);
>> + return sz < 0 ? -1 : 0;
>> +}
>> +
>>
>
> This function needs a docstring, and I'd suggest calling it virUUIDReadFile or
> ReadFromFile
>
> But the idea looks fine to me, CCing Dan since he suggested the libvirtd.conf
> option
>
> - Cole
>
I agree with all the suggestions and address them in the next version. By the way
looks like the function that reads uuid from file is quite generic and could be
named virFileReadAllBuf for example (there is virFileReadAll but it allocates buffer
for data and check for overflows).
Nikolay
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list