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

Re: [libvirt] [PATCH] Add <filesystem> support to Qemu's attach_device



I had run the 2 first checks but not the valgrind check.Sorry for the
memleak, hopefully you catched it. Thanks for your help through the
whole process.

Matt

2014-02-06 Michal Privoznik <mprivozn redhat com>:
> On 06.02.2014 15:51, Teto wrote:
>>
>> These 2 patches should address your points. I've also used
>> VIR_APPEND_ELEMENT in another function (1st patch).
>>
>> At your service should you have any other comment.
>>
>> Matthieu Coudron
>>
> <snip/>
>>
>> 0001-Replaced-VIR_REALLOC_N-by-VIR_APPEND_ELEMENT-in-virD.patch
>>
>>
>>  From 9235e4874266644af8180512e9920c35cfb0f09a Mon Sep 17 00:00:00 2001
>> From: Matthieu Coudron<mattator gmail com>
>> Date: Thu, 6 Feb 2014 15:29:08 +0100
>> Subject: [PATCH 1/2] Replaced VIR_REALLOC_N by VIR_APPEND_ELEMENT in
>>   virDomainHostdevInsert so that the code gets shorter and more readable
>>
>> Signed-off-by: Matthieu Coudron<mattator gmail com>
>> ---
>>   src/conf/domain_conf.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 28e24f9..3f3822e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -9868,10 +9868,8 @@ virDomainChrTargetTypeToString(int deviceType,
>>   int
>>   virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr
>> hostdev)
>>   {
>> -    if (VIR_REALLOC_N(def->hostdevs, def->nhostdevs + 1) < 0)
>> -        return -1;
>> -    def->hostdevs[def->nhostdevs++]  = hostdev;
>> -    return 0;
>> +
>> +    return VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev);
>>   }
>>
>
> virDomainHostdevRemove could be changed as well :) I've taken a chance and
> did it.
>
>>   virDomainHostdevDefPtr
>> -- 1.8.3.2
>>
>>
>> 0002-filesystem-support-in-device-addition-removal-for-th.patch
>>
>>
>>  From ed71d16b697c5a17946e5bef9fb74e6ac5a52fb0 Mon Sep 17 00:00:00 2001
>> From: Matthieu Coudron<mattator gmail com>
>> Date: Thu, 6 Feb 2014 15:30:07 +0100
>> Subject: [PATCH 2/2] <filesystem> support in device addition/removal for
>> the
>>   Qemu driver
>>
>> This commit allows to attach/detach a <filesystem> device in qemu (which
>> would previously return an error).
>>
>> For this purpose I've introduced 2 new functions "virDomainFSInsert" and
>> "virDomainFSRemove" and
>>
>> added the necessary code in the qemu driver.
>>
>> It compares filesystems based on their "destination" folder. So if 2
>> filesystems share a same destination, they are considered equal and the
>> Qemu driver would reject a new insertion.
>>
>> Signed-off-by: Matthieu Coudron<mattator gmail com>
>> ---
>>   src/conf/domain_conf.c   | 30 ++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.h   |  3 +++
>>   src/libvirt_private.syms |  2 ++
>>   src/qemu/qemu_driver.c   | 30 ++++++++++++++++++++++++++++++
>>   4 files changed, 65 insertions(+)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3f3822e..9e75b21 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -17931,6 +17931,36 @@ virDiskNameToBusDeviceIndex(virDomainDiskDefPtr
>> disk,
>>
>>       return 0;
>>   }
>>
>> +
>> +int
>> +virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs)
>> +{
>> +
>> +    return VIR_APPEND_ELEMENT(def->fss, def->nfss, fs);
>> +}
>> +
>> +virDomainFSDefPtr
>> +virDomainFSRemove(virDomainDefPtr def, size_t i)
>> +{
>> +    virDomainFSDefPtr fs = def->fss[i];
>> +
>> +    if (def->nfss > 1) {
>> +
>> +        memmove(def->fss + i,
>> +                def->fss + i + 1,
>> +                sizeof(*def->fss) *
>> +                (def->nfss - (i + 1)));
>> +        def->nfss--;
>> +        if (VIR_REALLOC_N(def->fss, def->nfss) < 0) {
>> +            /* ignore, harmless */
>> +        }
>> +    } else {
>> +        VIR_FREE(def->fss);
>> +        def->nfss = 0;
>> +    }
>> +    return fs;
>> +}
>> +
>
>
> Again, we have VIR_DELETE_ELEMENT, but I've changed this too.
>
>>   virDomainFSDefPtr
>>   virDomainGetRootFilesystem(virDomainDefPtr def)
>>   {
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index d8f2e49..9acb105 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2555,7 +2555,10 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr
>> disk,
>>
>>                                   int *devIdx);
>>
>>   virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def);
>> +int virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs);
>>   int virDomainFSIndexByName(virDomainDefPtr def, const char *name);
>> +virDomainFSDefPtr virDomainFSRemove(virDomainDefPtr def, size_t i);
>>
>> +
>>   int virDomainVideoDefaultType(const virDomainDef *def);
>>   int virDomainVideoDefaultRAM(const virDomainDef *def, int type);
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index c5a7637..2c9536a 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -221,6 +221,8 @@ virDomainFeatureStateTypeFromString;
>>   virDomainFeatureStateTypeToString;
>>   virDomainFSDefFree;
>>   virDomainFSIndexByName;
>> +virDomainFSInsert;
>> +virDomainFSRemove;
>>
>>   virDomainFSTypeFromString;
>>   virDomainFSTypeToString;
>>   virDomainFSWrpolicyTypeFromString;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 38a48db..8d7b228 100644
>>
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -6606,6 +6606,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr
>> qemuCaps,
>>       virDomainHostdevDefPtr hostdev;
>>       virDomainLeaseDefPtr lease;
>>       virDomainControllerDefPtr controller;
>> +    virDomainFSDefPtr fs;
>>
>>       switch (dev->type) {
>>       case VIR_DOMAIN_DEVICE_DISK:
>> @@ -6687,6 +6688,20 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr
>> qemuCaps,
>>
>>           dev->data.chr = NULL;
>>           break;
>>
>> +    case VIR_DOMAIN_DEVICE_FS:
>> +        fs = dev->data.fs;
>> +        if (virDomainFSIndexByName(vmdef, fs->dst) >= 0) {
>> +            virReportError(VIR_ERR_OPERATION_INVALID,
>> +                         "%s", _("Target already exists"));
>> +            return -1;
>> +        }
>> +
>> +        if (virDomainFSInsert(vmdef, fs) < 0) {
>> +            return -1;
>> +        }
>
>
> We don't tend to enclose a single line in brackets.
>
>
>> +        dev->data.fs = NULL;
>
>
> While this is okay, because virDomainFSInsert steals the object ...
>
>> +        break;
>> +
>>       default:
>>            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>                           _("persistent attach of device '%s' is not
>> supported"),
>> @@ -6707,6 +6722,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>>       virDomainLeaseDefPtr lease, det_lease;
>>       virDomainControllerDefPtr cont, det_cont;
>>       virDomainChrDefPtr chr;
>> +    virDomainFSDefPtr fs;
>>       int idx;
>>       char mac[VIR_MAC_STRING_BUFLEN];
>>
>> @@ -6783,6 +6799,20 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
>>
>>           dev->data.chr = NULL;
>>           break;
>>
>> +    case VIR_DOMAIN_DEVICE_FS:
>> +        fs = dev->data.fs;
>> +        idx = virDomainFSIndexByName(vmdef, fs->dst);
>> +        if (idx < 0) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                           _("no matching filesystem device was found"));
>>
>> +            return -1;
>> +        }
>> +
>> +        fs = virDomainFSRemove(vmdef, idx);
>> +        virDomainFSDefFree(fs);
>>
>> +        dev->data.fs = NULL;
>
>
> ... setting to NULL here, will lead to a memleak.
>
>> +        break;
>> +
>>       default:
>>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>                          _("persistent detach of device '%s' is not
>> supported"),
>> -- 1.8.3.2
>>
>
> Despite small nits, I'm givin ACK, fixing the issues and pushing.
> Congratulations on your first libvirt patches!
>
> Michal
>


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