Re: [Xen-devel] [libvirt] [PATCH V2 4/4] libxl: add support for rbd qdisk

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 02/22/2016 07:35 AM, Ján Tomko wrote:
> On Wed, Feb 17, 2016 at 05:33:45PM -0700, Jim Fehlig wrote:
>> xl/libxl already supports qemu's network-based block backends
>> such as nbd and rbd. libvirt has supported configuring such
>> <disk>s for long time too. This patch adds support for rbd
>> disks in the libxl driver by generating a rbd device URL from
>> the virDomainDiskDef object. The URL is passed to libxl via the
>> pdev_path field of libxl_device_disk struct. libxl then passes
>> the URL to qemu for cosumption by the rbd backend.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
>> ---
>>  src/libxl/libxl_conf.c | 192 
>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 191 insertions(+), 1 deletion(-)
>>
> ACK with the whitespace fix.
>
>> +
>> +static int
>> +libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
>> +{
>> +    virConnectPtr conn = NULL;
>> +    char *secret = NULL;
>> +    char *username = NULL;
>> +    int ret = -1;
>> +
>> +    *srcstr = NULL;
>> +    if (src->auth && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
>> +        const char *protocol = 
>> virStorageNetProtocolTypeToString(src->protocol);
>> +
>> +        username = src->auth->username;
>> +        if (!(conn = virConnectOpen("xen:///system")))
>> +            goto cleanup;
>> +
> Opening a connection feels out of place in this function, but I see it's
> already done for NICs.
>
> It would be nice to reuse it as is done in the qemu driver.

Heh, this has turned out to be a PITA. Something like the attached patch works,
but it still has the virConnectOpen in the libxlBuildDomainConfig call path. I
would prefer to use the virConnect object associated with the request, instead
of opening one in this code. I have some old, dusty patches (originally started
by danpb) that provide unit tests for libxlBuildDomainConfig. E.g. domXML ->
virDomainDef -> libxl_domain_config -> json representation -> compare with
expected json. Code in this path attempting to open "xen:///" connections is
likely to break many 'make check' setups :-).

But fixing this is not easy given the current code structure. There is one place
in particular that is rather troublesome - reboot. libxlDomainStart, and hence
libxlBuildDomainConfig, are called when a reboot event is received from libxl
(e.g. 'shutdown -r now' within a VM). As you might guess, there is no virConnect
object associated with such request. The code needs reworked quite a bit to
handle accessing a virConnect object while building a libxl_domain_config
object. I'll work on this when dusting off the unit test patches, although it is
not high in my queue. Maybe it would make a good libvirt GSoC project :-).

Regards,
Jim

Attachment: 0001-libxl-use-a-common-virConnect-object.patch
Description: Text Data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel