On Thu, Oct 15, 2009 at 12:52:49PM -0400, Dave Allan wrote:
> diff --git a/configure.in b/configure.in
> index 2f9db72..25ed55f 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -1640,7 +1640,7 @@ test "$enable_shared" = no && lt_cv_objdir=.
> LV_LIBTOOL_OBJDIR=${lt_cv_objdir-.}
> AC_SUBST([LV_LIBTOOL_OBJDIR])
>
> -dnl HAL or DeviceKit library for host device enumeration
> +dnl HAL, DeviceKit, or libudev library for host device enumeration
FYI, in the final submission, you should delete the DeviceKit driver
code entirely. It has never worked fully, and it is deprecated by its
own author in favour of udev.
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index f09f814..2322819 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -325,6 +325,9 @@ char *virNodeDeviceDefFormat(virConnectPtr conn,
> data->usb_if.subclass);
> virBufferVSprintf(&buf, " <protocol>%d</protocol>\n",
> data->usb_if.protocol);
> + if (data->usb_if.interface_name)
> + virBufferVSprintf(&buf, " <interface_name>%s</interface_name>\n",
> + data->usb_if.interface_name);
> if (data->usb_if.description)
> virBufferVSprintf(&buf, " <description>%s</description>\n",
> data->usb_if.description);
> @@ -394,10 +397,19 @@ char *virNodeDeviceDefFormat(virConnectPtr conn,
> "</media_available>\n", avl ? 1 : 0);
> virBufferVSprintf(&buf, " <media_size>%llu</media_size>\n",
> data->storage.removable_media_size);
> - virBufferAddLit(&buf, " </capability>\n");
> + virBufferVSprintf(&buf, " <logical_block_size>%llu"
> + "</logical_block_size>\n",
> + data->storage.logical_block_size);
> + virBufferVSprintf(&buf, " <num_blocks>%llu</num_blocks>\n",
> + data->storage.num_blocks);
> } else {
> virBufferVSprintf(&buf, " <size>%llu</size>\n",
> data->storage.size);
> + virBufferVSprintf(&buf, " <logical_block_size>%llu"
> + "</logical_block_size>\n",
> + data->storage.logical_block_size);
> + virBufferVSprintf(&buf, " <num_blocks>%llu</num_blocks>\n",
> + data->storage.num_blocks);
> }
> if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE)
> virBufferAddLit(&buf,
> @@ -1239,6 +1251,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
> VIR_FREE(data->usb_dev.vendor_name);
> break;
> case VIR_NODE_DEV_CAP_USB_INTERFACE:
> + VIR_FREE(data->usb_if.interface_name);
> VIR_FREE(data->usb_if.description);
> break;
> case VIR_NODE_DEV_CAP_NET:
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 29a4d43..95910c5 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -101,6 +101,7 @@ struct _virNodeDevCapsDef {
> unsigned function;
> unsigned product;
> unsigned vendor;
> + unsigned class;
> char *product_name;
> char *vendor_name;
> } pci_dev;
> @@ -117,10 +118,12 @@ struct _virNodeDevCapsDef {
> unsigned _class; /* "class" is reserved in C */
> unsigned subclass;
> unsigned protocol;
> + char *interface_name;
> char *description;
> } usb_if;
> struct {
> char *address;
> + unsigned address_len;
> char *ifname;
> enum virNodeDevNetCapType subtype; /* LAST -> no subtype */
> } net;
> @@ -139,6 +142,8 @@ struct _virNodeDevCapsDef {
> } scsi;
> struct {
> unsigned long long size;
> + unsigned long long num_blocks;
> + unsigned long long logical_block_size;
> unsigned long long removable_media_size;
> char *block;
> char *bus;
Ah, interesting - you're adding some more properties to the config struct.
Can you split this out into a separate patch - it'd be better for source
history to apply it separately.
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 14b3098..f3bd45d 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -70,9 +70,12 @@ static int update_caps(virNodeDeviceObjPtr dev)
> }
>
>
> -#ifdef __linux__
> -static int update_driver_name(virConnectPtr conn,
> - virNodeDeviceObjPtr dev)
> +#if defined (__linux__) && defined (HAVE_HAL)
> +/* XXX Why does this function exist? Are there devices that change
> + * their drivers while running? Under libudev, most devices seem to
> + * provide their driver name as a property "DRIVER" */
The problem is two fold
- HAL does not include any info about driver names
- The driver name may change during PCI/USB device attachment
to guest domains - ie from 'e1000' to 'pcistub' or even NULL
There is no useful event from HAL that we can use to refresh the
driver name to cope with point 2, so I took the hack approach of
just refreshing it everytime since it is a cheap operation.
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> new file mode 100644
> index 0000000..32fde52
> --- /dev/null
> +++ b/src/node_device/node_device_udev.c
> @@ -0,0 +1,800 @@
> +/*
> + * node_device_udev.c: node device enumeration - libudev implementation
> + *
> + * Copyright (C) 2009 Red Hat
> + *
> + * 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; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Dave Allan <dallan@xxxxxxxxxx>
> + */
> +
> +#include <config.h>
> +#include <libudev.h>
> +
> +#include "node_device_udev.h"
> +#include "virterror_internal.h"
> +#include "node_device_conf.h"
> +#include "node_device_driver.h"
> +#include "driver.h"
> +#include "datatypes.h"
> +#include "logging.h"
> +#include "memory.h"
> +#include "util.h"
> +#include "buf.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NODEDEV
> +
> +static virDeviceMonitorStatePtr driverState = NULL;
> +
> +/* This function allocates memory from the heap for the property
> + * value. That memory must be later freed by some other code. */
> +static int udevGetDeviceProperty(struct udev_device *udev_device,
> + const char *property_key,
> + char **property_value)
> +{
> + const char *udev_value = NULL;
> + int ret = -1;
> +
> + udev_value = udev_device_get_property_value(udev_device, property_key);
> + if (udev_value == NULL) {
> + VIR_ERROR(_("udev reports property '%s' does not exist"),
> + property_key);
> + goto out;
> + }
> +
> + /* If this allocation is changed, the comment at the beginning
> + * of the function must also be changed. */
> + *property_value = strdup(udev_value);
> + if (*property_value == NULL) {
> + VIR_ERROR(_("Failed to allocate memory for "
> + "property '%s' on device '%s'"),
> + property_key, udev_device_get_sysname(udev_device));
> + goto out;
> + }
> +
> + ret = 0;
> +
> +out:
> + if (ret != 0) {
> + VIR_ERROR(_("Failed to get udev property '%s' for device '%s'"),
> + property_key, udev_device_get_sysname(udev_device));
> + }
> +
> + return ret;
> +}
I think the error handling needs a little tweak - currently this treats
missing properties in the same way as real errors like OOM. I think its
calling conversion could be better as
- Success: return 0, property_value is non-NULL
- Missing: return 0, property_value is NULL
- Error: return -1, property_vlue is NULL, virReportError... called
and have the callers obviously check for the -1 case, but ignore the
missing property case (unless its a required property of course)
> +static int udevProcessPCI(struct udev_device *device,
> + virNodeDeviceDefPtr def)
> +{
> + const char *devpath = NULL;
> + int ret = -1;
> +
> + devpath = udev_device_get_devpath(device);
> +
> + char *p = strrchr(devpath, '/');
> + if (p) {
> + virStrToLong_ui(p+1, &p, 16, &def->caps->data.pci_dev.domain);
> + virStrToLong_ui(p+1, &p, 16, &def->caps->data.pci_dev.bus);
> + virStrToLong_ui(p+1, &p, 16, &def->caps->data.pci_dev.slot);
> + virStrToLong_ui(p+1, &p, 16, &def->caps->data.pci_dev.function);
Need to check whether the returned 'p' is NULL here & also for the return
status
> +/* XXX I don't fully understand the properties here. Advice needed. */
Any particular question ?
> +static int udevProcessUSBInterface(struct udev_device *device,
> + virNodeDeviceDefPtr def)
> +{
> + int ret = 0;
> + char *tmp = NULL, *p = NULL;
> +
> + udevGetStringProperty(device,
> + "INTERFACE",
> + &def->caps->data.usb_if.interface_name);
> +
> + tmp = strdup(def->caps->data.usb_if.interface_name);
> +
> + p = strrchr(tmp, '/');
> + if (p) {
> + *p = '\0';
> + virStrToLong_ui(tmp, NULL, 0, &def->caps->data.usb_if.number);
> + virStrToLong_ui(p+1, &p, 0, &def->caps->data.usb_if._class);
> + virStrToLong_ui(p+1, &p, 0, &def->caps->data.usb_if.subclass);
> + }
> +
> + VIR_FREE(tmp);
> + return ret;
> +}
> +
> +
> +static int udevProcessSCSIHost(struct udev_device *device ATTRIBUTE_UNUSED,
> + virNodeDeviceDefPtr def)
> +{
> + int ret = 0;
> + char *filename = NULL;
> +
> + filename = basename(def->name);
> +
> + if (STRPREFIX(filename, "host")) {
> + /* XXX There's really no better way to get the host #? */
None that I am aware of.
> + ret = virStrToLong_ui(filename + strlen("host"),
> + NULL,
> + 0,
> + &def->caps->data.scsi_host.host);
> + check_fc_host(&def->caps->data);
> + check_vport_capable(&def->caps->data);
> + }
> +
> + return ret;
> +}
> +
> +
> +static int udevProcessSCSIDevice(struct udev_device *device ATTRIBUTE_UNUSED,
> + virNodeDeviceDefPtr def)
> +{
> + int ret = 0;
> + char *filename = NULL, *p = NULL;
> +
> + filename = basename(def->name);
> +
> + virStrToLong_ui(filename, &p, 10, &def->caps->data.scsi.host);
> + virStrToLong_ui(p+1, &p, 10, &def->caps->data.scsi.bus);
> + virStrToLong_ui(p+1, &p, 10, &def->caps->data.scsi.target);
> + virStrToLong_ui(p+1, &p, 10, &def->caps->data.scsi.lun);
Should also check for return status & p != NULL here.
> +static int udevProcessStorage(struct udev_device *device,
> + virNodeDeviceDefPtr def)
> +{
> + int ret = -1;
> +
> + def->caps->data.storage.block = strdup(udev_device_get_devnode(device));
> + udevGetStringProperty(device, "DEVNAME", &def->caps->data.storage.block);
> + udevGetStringProperty(device, "ID_BUS", &def->caps->data.storage.bus);
> + udevGetStringProperty(device, "ID_SERIAL", &def->caps->data.storage.serial);
> + udevGetStringSysfsAttr(device, "device/vendor", &def->caps->data.storage.vendor);
> + udevStripSpaces(def->caps->data.storage.vendor);
> + udevGetStringSysfsAttr(device, "device/model", &def->caps->data.storage.model);
> + udevStripSpaces(def->caps->data.storage.model);
> + /* XXX Is there an equivalent of the HAL hotpluggable property?
> + * Is the hotpluggable property still relevant? */
Probably best to look at the HAL source to see what criteria it uses
for setting hotpluggable.
> +
> + if (udevGetStringProperty(device,
> + "ID_TYPE",
> + &def->caps->data.storage.drive_type) != 0) {
> + /* If udev doesn't have it, perhaps we can guess it. */
> + if (udevKludgeStorageType(def) != 0) {
> + goto out;
> + }
> + }
> +
> + /* NB: drive_type has changed from HAL; now it's "cd" instead of "cdrom" */
> + if (STREQ(def->caps->data.storage.drive_type, "cd")) {
> + ret = udevProcessCDROM(device, def);
> + } else if (STREQ(def->caps->data.storage.drive_type, "disk")) {
> + ret = udevProcessDisk(device, def);
> + } else {
> + goto out;
> + }
> +
> +out:
> + return ret;
> +}
It is all starting to look good. The main thing that I'd say were a
blocker is the parent/child relationship hookup.
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