Re: [libvirt] PATCH: 8/12:Interal driver API for node devices

2008-11-14 Thread Mark McLoughlin 
On Thu, 2008-11-13 at 17:31 +, Daniel P. Berrange wrote:
 This is the basic internal driver support code for the node device APIs.
 The actual registration of the drivers was pushed to a later patch to
 allow this to compile on its own  thus be fully bisectable.
 
 Daniel
 
 diff -r 6812c3044043 src/Makefile.am
 --- a/src/Makefile.am Wed Nov 12 21:11:46 2008 +
 +++ b/src/Makefile.am Wed Nov 12 21:11:51 2008 +
 @@ -132,6 +132,10 @@
   storage_driver.h storage_driver.c   \
   storage_backend.h storage_backend.c
  
 +# Network driver generic impl APIs

Typo

 +NODE_DEVICE_CONF_SOURCES =   \
 + node_device_conf.c node_device_conf.h
 +
...
 diff -r 6812c3044043 src/node_device_conf.c
 --- /dev/null Thu Jan 01 00:00:00 1970 +
 +++ b/src/node_device_conf.c  Wed Nov 12 21:11:51 2008 +
...
 +char *virNodeDeviceDefFormat(virConnectPtr conn,
 + const virNodeDeviceDefPtr def)
 +{
 +virBuffer buf = VIR_BUFFER_INITIALIZER;
 +virNodeDevCapsDefPtr caps = def-caps;
 +char *tmp;
 +
 +virBufferAddLit(buf, device\n);
 +virBufferEscapeString(buf,   name%s/name\n, def-name);
 +
 +if (def-parent)
 +virBufferEscapeString(buf,   parent%s/parent\n, def-parent);
 +
 +for (caps = def-caps; caps; caps = caps-next) {
 +char uuidstr[VIR_UUID_STRING_BUFLEN];
 +union _virNodeDevCapData *data = caps-data;
 +
 +virBufferVSprintf(buf,   capability type='%s'\n,
 +  virNodeDevCapTypeToString(caps-type));
 +switch (caps-type) {
...
 +case VIR_NODE_DEV_CAP_NET:
 +virBufferVSprintf(buf, interface%s/interface\n,
 +  data-net.interface);
 +if (data-net.address)
 +virBufferVSprintf(buf, address%s/address\n,
 +  data-net.address);
 +if (data-net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
 +const char *subtyp =
 +virNodeDevNetCapTypeToString(data-net.subtype);
 +virBufferVSprintf(buf, capability type='%s'\n, 
 subtyp);
 +switch (data-net.subtype) {
 +case VIR_NODE_DEV_CAP_NET_80203:
 +virBufferVSprintf(buf,
 +
 mac_address%012llx/address\n,
 +  data-net.data.ieee80203.mac_address);
 +break;
 +case VIR_NODE_DEV_CAP_NET_80211:
 +virBufferVSprintf(buf,
 +
 mac_address%012llx/address\n,
 +  data-net.data.ieee80211.mac_address);
 +break;
 +case VIR_NODE_DEV_CAP_NET_LAST:
 +/* Keep dumb compiler happy */
 +break;
 +}
 +virBufferAddLit(buf, /capability\n);

This all seems a bit odd.

We've listed the mac address once already, so why do it again in a hex format?

And I wouldn't think of 802.3 vs 802.11 as a network device capability, but 
more like it's type - they're mutually exclusive, right?

Also, we have:

  device
...
capability type='net'
  ...
  capability type='80203'
mac_address001320f74a06/address
  /capability
/capability
  /device

i.e. two different capability semantics?

 +}
 +break;
 +case VIR_NODE_DEV_CAP_BLOCK:
 +virBufferVSprintf(buf, device%s/device\n,
 +  data-block.device);

Two nested device nodes:

device
  ...
  capability type='block'
device/dev/sda1/device
  /capability
/device

How about path or device_path ?

 +break;
 +case VIR_NODE_DEV_CAP_SCSI_HOST:
 +virBufferVSprintf(buf, host%d/host\n,
 +  data-scsi_host.host);
 +break;
 +case VIR_NODE_DEV_CAP_SCSI:
 +virBufferVSprintf(buf, host%d/host\n, 
 data-scsi.host);
 +virBufferVSprintf(buf, bus%d/bus\n, data-scsi.bus);
 +virBufferVSprintf(buf, target%d/target\n,
 +  data-scsi.target);
 +virBufferVSprintf(buf, lun%d/lun\n, data-scsi.lun);
 +if (data-scsi.type)
 +virBufferVSprintf(buf, type%s/type\n,
 +  data-scsi.type);
 +break;
 +case VIR_NODE_DEV_CAP_STORAGE:
 +if (data-storage.bus)
 +virBufferVSprintf(buf, bus%s/bus\n,
 +  data-storage.bus);
 +if (data-storage.drive_type)
 +virBufferVSprintf(buf, drive_type%s/drive_type\n,
 +  data-storage.drive_type);
 +if (data-storage.originating_device)
 +

Re: [libvirt] PATCH: 8/12:Interal driver API for node devices

2008-11-14 Thread David Lively 
On Fri, 2008-11-14 at 13:20 +, Mark McLoughlin wrote:
 On Thu, 2008-11-13 at 17:31 +, Daniel P. Berrange wrote:
 
  +virBufferVSprintf(buf,   capability type='%s'\n,
  +  virNodeDevCapTypeToString(caps-type));
  +switch (caps-type) {
 ...
  +case VIR_NODE_DEV_CAP_NET:
  +virBufferVSprintf(buf, interface%s/interface\n,
  +  data-net.interface);
  +if (data-net.address)
  +virBufferVSprintf(buf, address%s/address\n,
  +  data-net.address);
  +if (data-net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
  +const char *subtyp =
  +virNodeDevNetCapTypeToString(data-net.subtype);
  +virBufferVSprintf(buf, capability type='%s'\n, 
  subtyp);
  +switch (data-net.subtype) {
  +case VIR_NODE_DEV_CAP_NET_80203:
  +virBufferVSprintf(buf,
  +
  mac_address%012llx/address\n,
  +  
  data-net.data.ieee80203.mac_address);
  +break;
  +case VIR_NODE_DEV_CAP_NET_80211:
  +virBufferVSprintf(buf,
  +
  mac_address%012llx/address\n,
  +  
  data-net.data.ieee80211.mac_address);
  +break;
  +case VIR_NODE_DEV_CAP_NET_LAST:
  +/* Keep dumb compiler happy */
  +break;
  +}
  +virBufferAddLit(buf, /capability\n);
 
 This all seems a bit odd.
 
 We've listed the mac address once already, so why do it again in a hex format?

Agreed.  It's basically just reflecting the HAL attributes.  But I have
to admit I find a lot of HAL rather non-intuitive ...  I have no
particular attachment to any of this node device description XML.  I
really liked Daniel's suggestion that this be based on a subset of HAL,
and have tried to do that as much as possible.  I don't really care what
it's based on, but it sure would be nice to avoid inventing yet another
XML device representation (hence the attraction to HAL).  

But the more I've gotten to know HAL, the less I've liked the idea.  I'm
not aware of an alternative that doesn't involve inventing our own.
Perhaps there's something suitable in CIM??  I'm a CIM-dummy, so I have
no idea ...   This is related to the device naming issue as well.  I
share all of Dan B's concerns here ...

 
 And I wouldn't think of 802.3 vs 802.11 as a network device capability, but 
 more like it's type - they're mutually exclusive, right?
 
 Also, we have:
 
   device
 ...
 capability type='net'
   ...
   capability type='80203'
 mac_address001320f74a06/address
   /capability
 /capability
   /device
 
 i.e. two different capability semantics?
 

Again, just reflecting HAL.  But I agree it seems bizarre.

  +}
  +break;
  +case VIR_NODE_DEV_CAP_BLOCK:
  +virBufferVSprintf(buf, device%s/device\n,
  +  data-block.device);
 
 Two nested device nodes:
 
 device
   ...
   capability type='block'
 device/dev/sda1/device
   /capability
 /device
 
 How about path or device_path ?

This one doesn't bother me so much, though granted the two device tags
are talking about completely different things (clear to me from the
context, but perhaps violating some proper XML design rules?).

  +if (data-storage.originating_device)
  +virBufferVSprintf(buf,
  +  originating_device%s
  +  /originating_device\n,
  +  data-storage.originating_device);
 
 This originating_device thing sounds strange, and I don't think we
 implement it yet. Leave it out for now?

Fine with me.  I don't know what it is -- just sounded generally useful.
If it's not widely implemented, we probably shouldn't bother supporting
it.

  +if (data-storage.flags  VIR_NODE_DEV_CAP_STORAGE_REMOVABLE) {
  +int avl = data-storage.flags 
  +VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE;
  +virBufferAddLit(buf, capability 
  type='removable'\n);
  +virBufferVSprintf(buf,
  +media_available%d
  +  /media_available\n, avl ? 1 : 0);
  +virBufferVSprintf(buf,   
  media_size%llu/media_size\n,
  +  data-storage.removable_media_size);
  +virBufferAddLit(buf, /capability\n);
  +} else {
  +virBufferVSprintf(buf, size%llu/size\n,
  +  data-storage.size);
  +

Re: [libvirt] PATCH: 8/12:Interal driver API for node devices

2008-11-13 Thread Daniel P. Berrange 
This is the basic internal driver support code for the node device APIs.
The actual registration of the drivers was pushed to a later patch to
allow this to compile on its own  thus be fully bisectable.

Daniel

diff -r 6812c3044043 src/Makefile.am
--- a/src/Makefile.am   Wed Nov 12 21:11:46 2008 +
+++ b/src/Makefile.am   Wed Nov 12 21:11:51 2008 +
@@ -132,6 +132,10 @@
storage_driver.h storage_driver.c   \
storage_backend.h storage_backend.c
 
+# Network driver generic impl APIs
+NODE_DEVICE_CONF_SOURCES = \
+   node_device_conf.c node_device_conf.h
+
 STORAGE_DRIVER_FS_SOURCES =\
storage_backend_fs.h storage_backend_fs.c
 
@@ -167,7 +171,8 @@
$(DRIVER_SOURCES)   \
$(DOMAIN_CONF_SOURCES)  \
$(NETWORK_CONF_SOURCES) \
-   $(STORAGE_CONF_SOURCES)
+   $(STORAGE_CONF_SOURCES) \
+   $(NODE_DEVICE_CONF_SOURCES)
 
 if WITH_TEST
 if WITH_DRIVER_MODULES
diff -r 6812c3044043 src/node_device_conf.c
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/src/node_device_conf.cWed Nov 12 21:11:51 2008 +
@@ -0,0 +1,425 @@
+/*
+ * node_device_conf.c: config handling for node devices
+ *
+ * Copyright (C) 2008 Virtual Iron Software, Inc.
+ * Copyright (C) 2008 David F. Lively
+ *
+ * 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: David F. Lively [EMAIL PROTECTED]
+ */
+
+#include config.h
+
+#include unistd.h
+#include errno.h
+
+#include virterror_internal.h
+#include memory.h
+
+#include node_device_conf.h
+#include memory.h
+#include xml.h
+#include util.h
+#include buf.h
+#include uuid.h
+
+
+VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST,
+  system,
+  pci,
+  usb_device,
+  usb,
+  net,
+  block,
+  scsi_host,
+  scsi,
+  storage);
+
+VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST,
+  80203,
+  80211);
+
+
+#define virNodeDeviceLog(msg...) fprintf(stderr, msg)
+
+virNodeDeviceObjPtr virNodeDeviceFindByName(const virNodeDeviceObjListPtr devs,
+const char *name)
+{
+unsigned int i;
+
+for (i = 0; i  devs-count; i++)
+if (STREQ(devs-objs[i]-def-name, name))
+return devs-objs[i];
+
+return NULL;
+}
+
+
+void virNodeDeviceDefFree(virNodeDeviceDefPtr def)
+{
+virNodeDevCapsDefPtr caps;
+
+if (!def)
+return;
+
+VIR_FREE(def-name);
+VIR_FREE(def-parent);
+
+caps = def-caps;
+while (caps) {
+virNodeDevCapsDefPtr next = caps-next;
+virNodeDevCapsDefFree(caps);
+caps = next;
+}
+
+VIR_FREE(def);
+}
+
+void virNodeDeviceObjFree(virNodeDeviceObjPtr dev)
+{
+if (!dev)
+return;
+
+virNodeDeviceDefFree(dev-def);
+if (dev-privateFree)
+(*dev-privateFree)(dev-privateData);
+
+VIR_FREE(dev);
+}
+
+void virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
+{
+unsigned int i;
+for (i = 0 ; i  devs-count ; i++)
+virNodeDeviceObjFree(devs-objs[i]);
+VIR_FREE(devs-objs);
+devs-count = 0;
+}
+
+virNodeDeviceObjPtr virNodeDeviceAssignDef(virConnectPtr conn,
+   virNodeDeviceObjListPtr devs,
+   const virNodeDeviceDefPtr def)
+{
+virNodeDeviceObjPtr device;
+
+if ((device = virNodeDeviceFindByName(devs, def-name))) {
+virNodeDeviceDefFree(device-def);
+device-def = def;
+return device;
+}
+
+if (VIR_ALLOC(device)  0) {
+virNodeDeviceReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+return NULL;
+}
+
+device-def = def;
+
+if (VIR_REALLOC_N(devs-objs, devs-count+1)  0) {
+device-def = NULL;
+virNodeDeviceObjFree(device);
+virNodeDeviceReportError(conn, VIR_ERR_NO_MEMORY, NULL);
+return NULL;
+}
+devs-objs[devs-count++] = device;
+
+return device;
+
+}
+
+void