Re: [Libvirt-cim] [PATCH 00/15] vlan extension

On 12/08/2011 09:38 AM, Eduardo Lima (Etrunko) wrote: > On 12/07/2011 07:18 AM, Wayne Xia wrote: >> These patches would do following things: building up the network >> system CIM >> model, building up basic framework and functions below to implement the >> configuration, add the bridge and vlan802.1.q configuration >> capabilities in >> CIM model and library below. >> Now this patch implement the functionalities with commandline >> style, which >> means it depends on command ip, ifconfig, brctl and /proc/net/vlan/. >> In this >> way it is very sensitive to these command's output format. Maybe >> another >> implemetion could avoid that in the future. > What exactly do you need with those commands. If we're talking about > listing network interfaces, obtaining IP/MAC addresses and like, this > all should be done via standard system calls instead of parsing command > line outputs. This is the basics of UNIX IPC mechanism. For instance see > socket(7), ip(7), netdevice(7) and related man pages, especially the > Ioctls sections. > Eduardo is correct in that we should attempt to use OS APIs, and libvirt APIs of course, whenever possible. Output parsing is error prone and slow and can make decreases portability.

>> Since libvirt-cim is not a daemon program that would always be >> brought up >> when the emulator was up, so there lacks a mechnism to save the >> configuration >> and set them when system reboot, so these patch uses system directory >> /etc/sysconfig/network-script/ >> to store the settings. Of course, any changes (add/create/modify) need to apply to the system real time and next boot. If there are no APIs to make this changes persistent across boots then we'll need to do something like this but we must be very careful never to break anything. For example, we should copy everything line-by-line into a tmp file and only modify the lines we must. If that succeeds, copy the tmp file to the appropriate network script file and then remove the tmp file (in that order).

>> The things above means that a linux kernel later than 2.6 and RedHat >> distribution is needed. Other distriution may work but not tested. If >> the implemention goes to libvirt the restrict would be removed. > Depending on a specific distro is very very bad. Again, what kind of > configuration are you trying to store? Could the infostore > implementation in libvirt-cim which could be used? > > And this line about implementation going to libvirt really makes me sad. > IMHO, if there is work to be done in libvirt, so why don't start there > instead of implementing everything in house and having to do everything > once again when the necessary bits are there? This is a bit of a grey area. I'm not sure libvirt would be interested in managing the hosts NIC and bridges. I think that might fall outside libvirt. That implies libvirt-cim in this case since there are several use cases were a CIM client needs to create a VLAN, create a VM, and then link the two.

> > [snip] > >> Wayne Xia (15): >> vlan extension - makefile change >> vlan extension - CIM model - add class VESS >> vlan extension - CIM model - add class VESSSD >> vlan extension - CIM model - add class EthernetPort >> vlan extension - CIM model - add class >> EthernetPortAllocationSettingData >> vlan extension - CIM model - add association Net_SettingsDeineState >> vlan extension - CIM model - add association Net_SystemDevice >> vlan extension - CIM model - add association Net_VESSSDComponent >> vlan extension - CIM model - add association Net_ElementSettingData >> vlan extension - CIM model - add core class VESSMS >> vlan extension - CIM model - add help functions >> vlan extension - function lib - add the API >> vlan extension - function lib - add the core structure >> vlan extension - function lib - add the helper functions >> vlan extension - function lib - add the implemention of commandline >> > For the series, looks like you sent the patches in the opposite order, > from last to first. If I apply the fist patch the build breaks. The > first review for this whole series is a -1 only because of that. > > I think it would help a lot if you publish your code in gitorious for > instance. I have set up the mirrors for libvirt-cim repositories there: > > http://gitorious.org/libvirt-cim > > Finally, for future cases which involve huge amount of work, like this > one, *please please please* be gentle with us and avoid one big code > drop like this one. Small and incremental changes makes the life of > everyone, including yours, easier. :) > > Best regards, Etrunko. > >> Makefile.am | 16 +- >> libxkutil/Makefile.am | 11 +- >> libxkutil/host_network_API.c | 150 ++ >> libxkutil/host_network_API.h | 32 + >> libxkutil/host_network_basic.c | 639 +++++++ >> libxkutil/host_network_basic.h | 166 ++ >> libxkutil/host_network_error.h | 28 + >> libxkutil/host_network_helper.c | 659 +++++++ >> libxkutil/host_network_helper.h | 192 ++ >> libxkutil/host_network_implement_cmdline.c | 2002 >> ++++++++++++++++++++ >> libxkutil/host_network_implement_cmdline.h | 55 + >> libxkutil/network_model.c | 466 +++++ >> libxkutil/network_model.h | 105 + > P.S.: > I haven't had the chance to review the patches yet. But I have a sudden > cold feeling in my spine by looking at these first stats. >