From: Ian Jackson <ian.jackson@eu.citrix.com>
To: Juergen Gross <jgross@suse.com>
Cc: wei.liu2@citrix.com, xen-devel@lists.xen.org
Subject: Re: [PATCH 0/3] libxl: add framework for device types
Date: Wed, 6 Jul 2016 12:04:32 +0100 [thread overview]
Message-ID: <22396.58816.598381.345930@mariner.uk.xensource.com> (raw)
In-Reply-To: <1466519091-29348-1-git-send-email-jgross@suse.com>
Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"):
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.
I think something like this is a jolly good idea. Thanks a lot!
The rough shape - a set of structs with what amount to method calls -
seems a good direction to be going in.
I have some comments/questions.
I saw this in 2/3:
> + for (i = 0; i < d_config->num_pcidevs; i++) {
> + rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> + if (rc < 0) {
> + LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> + goto out;
> + }
> + }
> +
And there is similar code in 3/3 for dtdevs. Could that be lifted
away somehow ? (You'd have to take some care about the types, sadly;
ie, I think libxl__device_pci_add might have to start to take a
void*; maybe some macros could make things typesafe?)
In 1/3:
> +struct libxl_device_type libxl__usbctrl_devtype = {
> + .type = "usbctrl",
> + .num_offset = offsetof(libxl_domain_config, num_usbctrls),
> + .add = libxl__add_usbctrls,
> +};
And then num_offset is used like this:
> + if (*(int *)((void *)d_config + dt->num_offset) > 0) {
This is a fine approach but I would prefer it if the there were a bit
more type safety, particularly in the parts that have to occur once
for each device type.
For example, there is nothing stopping one using this pattern with a
num_* field which is not an int.
Perhaps there should be a macro for generating the libxl_device_type
contents ? It could perhaps take `usbctrls' and make `num_usbctrls'
out of it using token pasting.
Also these structs should be static const, I think.
Thanks again!
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-07-06 11:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-21 14:24 [PATCH 0/3] libxl: add framework for device types Juergen Gross
2016-06-21 14:24 ` [PATCH 1/3] " Juergen Gross
2016-06-21 14:24 ` [PATCH 2/3] libxl: refactor domcreate_attach_pci() to use device type framework Juergen Gross
2016-06-21 14:24 ` [PATCH 3/3] libxl: refactor domcreate_attach_dtdev() " Juergen Gross
2016-07-04 10:26 ` [PATCH 0/3] libxl: add framework for device types Juergen Gross
2016-07-06 11:04 ` Ian Jackson [this message]
2016-07-06 12:40 ` Juergen Gross
2016-07-06 12:47 ` Ian Jackson
2016-07-06 12:55 ` Juergen Gross
2016-07-06 16:09 ` Ian Jackson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=22396.58816.598381.345930@mariner.uk.xensource.com \
--to=ian.jackson@eu.citrix.com \
--cc=jgross@suse.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).