On Mon, Oct 05, 2009 at 11:21:05AM -0400, Dave Allan wrote:
> Hi all,
>
> Here is a first cut at the node device backend. The only devices
> implemented are part of storage and PCI devices, but it's conceptually
> finished. The remaining work is implementing the other device
> properties and types. Before I go do that, I'd like to get feedback on
> my approach. Since libudev doesn't have the concept of capabilities the
> way HAL does, Chris Lalancette and I thought it made more sense and
> rendered much more readable code to implement each device type in a
> single function and avoid the indirection created by the caps_tbl
> structure containing a function pointer.
I think this approach is fine - there's absolutely no reason or need
to follow the capabilities table based lookup approach if it is not
a good match for udev. The only important thing is the resulting data,
not how we populate it :-)
> Monitoring of libudev for new devices is not implemented, but I'm not
> too concerned about that at this point. As Dan Berrange suggested on
> IRC, putting a call in the main loop to poll the monitor file descriptor
> will do the job.
Yep, its a minor detail compared to the main body of the code.
> +
> + /* These lines are somewhat more than 80 col, but I think the code
> + * is more readable if they aren't broken up. */
> + if (udevGetStringProperty(device, "DEVNAME", &def->caps->data.storage.block)) {
> + /* None of these properties are available through libudev...
> + udevGetStringProperty(device, "ID_MODEL", &def->caps->data.storage.model) ||
> + udevGetStringProperty(device, "ID_VENDOR", &def->caps->data.storage.vendor) ||
> + udevGetStringProperty(device, "ID_SERIAL", &def->caps->data.storage.serial)) {
> + */
Guess we need to read sysfs for those values.
> +static int udevProcessOneDevice(struct udev_device *device,
> + virNodeDeviceDefPtr def)
> +{
> + virNodeDeviceObjPtr dev = NULL;
> + const char *devtype = NULL, *devpath = NULL;
> + regex_t pci_rec;
> + int ret = -1;
> +
> + /* How to handle parent devices? Shall we change the meaning of
> + * the parent pointer for the udev backend? What does client code
> + * use the parent relationship for? We really want to preserve
> + * the behavior of the HAL backend, but that may be difficult in
> + * practice. */
> + //parent = udev_device_get_parent(device);
This raises a interesting question for us to consider. At this point
in time our schema defines a standard set of capabilities/attributes
we expose in the XML format.
We do not officially define anything about device naming, or about
device hierarchy.
It may well be useful for us to consider declaring an official naming
policy & hierachy and applying that consistently, rather than just
exposing the udev / HAL specific policies which vary according to the
phase of the moon.
FWIW, the most important current usecase for the hierarchy information
at the current time is in the area of device passthrough to guests.
ie, if you're about to disconnect PCI device 00.1c.3 from the host
OS drivers, you can check the hierarchy to make sure this PCI device
isn't the parent of the disk controller that holds your host OS'
root filesystem, or primary network interface, etc, etc.
> +/* Don't call this function directly, call virBuildPath instead. */
> +int virBuildPathInternal(char **path, ...)
I think it'd be
> +{
> + char *path_component = NULL;
> + va_list ap;
> + size_t used = 0;
> + int ret = 0;
> +
> + if (VIR_ALLOC_N(*path, PATH_MAX) != 0) {
> + ret = -1;
> + goto out;
> + }
It might be nice to use virBuffer for this function - I think it'd
be slightly less scary to read if the code doesn't need to include all
the buffer allocation / checking directly :-)
> +
> + va_start(ap, *path);
> +
> + while ((path_component = va_arg(ap, char *)) != NULL)
> + {
> + used += snprintf(*path + used, PATH_MAX - used, "%s/", path_component);
> +
> + /* From the snprintf man page:
> +
> + The functions snprintf() and vsnprintf() do not write more
> + than size bytes (including the trailing '\0'). If the
> + output was truncated due to this limit then the return
> + value is the number of characters (not including the
> + trailing '\0') which would have been written to the final
> + string if enough space had been available. Thus, a return
> + value of size or more means that the output was truncated.
> +
> + We therefore set used to the size of the buffer to avoid
> + the size being passed to snprintf from becoming negative.
> + */
> +
> + if (used > PATH_MAX) {
> + used = PATH_MAX;
> + }
> + }
> +
> + va_end(ap);
> +
> + *(*path + used - 1) = '\0'; /* kill trailing slash */
> +
> +out:
> + return ret;
> +}
> diff --git a/src/util/util.h b/src/util/util.h
> index dbfdc45..aa466af 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -244,4 +244,8 @@ char *virFileFindMountPoint(const char *type);
>
> void virFileWaitForDevices(virConnectPtr conn);
>
> +#define virBuildPath(path, ...) virBuildPathInternal(path, __VA_ARGS__, NULL)
> +/* Don't call virBuildPathInternal directly, use virBuildPath */
> +int virBuildPathInternal(char **path, ...);
Rather than using a macro here, you could just annotate the real
method with
__attribute__ ((sentinel))
which will make the compiler validate that all callers include a trailing
literal NULL. (This check is turned on with -Wformat, which we include by
default).
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list