Re: [PATCH v3 2/3] xen: write information about supported backends

[Juergen Gross <jgross@suse.com>]

On 10/05/16 18:21, Anthony PERARD wrote:
> On Fri, May 06, 2016 at 11:42:45AM +0200, Juergen Gross wrote:
>> Add a Xenstore directory for each supported pv backend. This will allow
>> Xen tools to decide which backend type to use in case there are
>> multiple possibilities.
>>
>> The information is added under
>> /local/domain/<backend-domid>/device-model/<domid>/backends
>> before the "running" state is written to Xenstore. Using a directory
>> for each backend enables us to add parameters for specific backends
>> in the future.
>>
>> This interface is documented in the Xen source repository in the file
>> docs/misc/qemu-backends.txt
>>
>> In order to reuse the Xenstore directory creation already present in
>> hw/xen/xen_devconfig.c move the related functions to
>> hw/xen/xen_backend.c where they fit better.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3: Added .backend_register function to XenDevOps in order to have a
>>     way to let the backend decide whether all prerequisites are met
>>     for support.
>>
>> V2: update commit message as requested by Stefano
>> ---
>>  hw/xen/xen_backend.c         | 60 ++++++++++++++++++++++++++++++++++++++++++++
>>  hw/xen/xen_devconfig.c       | 52 ++------------------------------------
>>  include/hw/xen/xen_backend.h |  2 ++
>>  3 files changed, 64 insertions(+), 50 deletions(-)
>>
>> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
>> index 60575ad..6d8b3a5 100644
>> --- a/hw/xen/xen_backend.c
>> +++ b/hw/xen/xen_backend.c
>> @@ -726,6 +772,20 @@ err:
>>  
>>  int xen_be_register(const char *type, struct XenDevOps *ops)
>>  {
>> +    char path[50];
>> +    int rc;
>> +
>> +    if (ops->backend_register) {
>> +        rc = ops->backend_register();
>> +        if (rc) {
>> +            return rc;
>> +        }
>> +    }
>> +
>> +    snprintf(path, sizeof (path), "device-model/%u/backends/%s", xen_domid,
>> +             type);
>> +    xenstore_mkdir(path, XS_PERM_READ);
> 
> Do you actually need the guest to be able to read those paths?

No, you are right.

>> diff --git a/hw/xen/xen_devconfig.c b/hw/xen/xen_devconfig.c
>> index 1f30fe4..b7d290d 100644
>> --- a/hw/xen/xen_devconfig.c
>> +++ b/hw/xen/xen_devconfig.c
>> @@ -5,54 +5,6 @@
>>  
>>  /* ------------------------------------------------------------- */
>>  
>> -struct xs_dirs {
>> -    char *xs_dir;
>> -    QTAILQ_ENTRY(xs_dirs) list;
>> -};
>> -static QTAILQ_HEAD(xs_dirs_head, xs_dirs) xs_cleanup = QTAILQ_HEAD_INITIALIZER(xs_cleanup);
>> -
>> -static void xen_config_cleanup_dir(char *dir)
>> -{
>> -    struct xs_dirs *d;
>> -
>> -    d = g_malloc(sizeof(*d));
>> -    d->xs_dir = dir;
>> -    QTAILQ_INSERT_TAIL(&xs_cleanup, d, list);
>> -}
>> -
>> -void xen_config_cleanup(void)
>> -{
>> -    struct xs_dirs *d;
>> -
>> -    QTAILQ_FOREACH(d, &xs_cleanup, list) {
>> -	xs_rm(xenstore, 0, d->xs_dir);
>> -    }
>> -}
>> -
>> -/* ------------------------------------------------------------- */
>> -
>> -static int xen_config_dev_mkdir(char *dev, int p)
>> -{
>> -    struct xs_permissions perms[2] = {{
>> -            .id    = 0, /* set owner: dom0 */
>> -        },{
>> -            .id    = xen_domid,
>> -            .perms = p,
>> -        }};
>> -
> 
> The function looks like it as been tailored to mkdir for config of
> backends. So it does not seems out of place.

It has been tailored for config of _devices_ by the backend.

I still think it would fit better now into xen_backend.c, OTOH in case
you like it better in xen_devconfig.c I won't argue against it.


Juergen