Re: [PATCH 2/4 V4] util: add virtkey

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/19/2011 09:04 AM, Daniel P. Berrange wrote:
On Thu, Jul 14, 2011 at 11:32:16AM +0800, Lai Jiangshan wrote:
Add virtkey lib for usage-improvment and keycode translating.
Add 4 internal API for the aim

+
+struct keycode {

+extern struct keycode virtKeycodes[];
+extern int virtKeycodesSize;

None of these three declarations need to be in the header file
since they are private impl details of the source fiel.

Well, as written in this patch, they are used in both virtkey.c, and by the generated virkeymaps.c. But, a better idea would be:

virkey.h: public functions (virKeycodeSetToString, virKeycodeSetFromString [both from VIR_ENUM_DECL], virKeycodeValueFromString, virKeycodeValueTranslate)

virkey.c:
#include "virkey.h"
#define VIR_KEY_INTERNAL
struct keycode {} ...
#include "virkeymap.h"
implement the public functions

virkeymap-gen.pl: generates virkeymap.h

virkeymap.h:
#ifndef VIR_KEY_INTERNAL
# error do not use this; it is not a public header
#endif
static struct keycode keycodes[] = {
...
}

that way, the generated file is not a separate c file, but a chunk of included code to complete the single virkey.c. See for example how remote_driver.c does a #include "remote_client_bodies.h" in the middle of the file, and make sure that the Makefile.am snippets for virkeymap.h (making sure it is in EXTRA_DIST and so forth) resemble the rules for remote_client_bodies.h.

I was debating about whipping this series into shape according to Dan's review comments, since you got acks on the other 3 out of 4, but the changes to patch 2 are looking to be significant enough to need a v5.

--
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]