Re: [libvirt] [PATCHv2 07/33] storage: Add support for access to files using provided uid/gid

On 05/22/2014 07:47 AM, Peter Krempa wrote: > To allow using the storage driver APIs to access files on various > storage sources in an universal fashion possibly on storage such as nfs s/an universal/a universal/ > with root squash we'll need to store the desired uid/gid in the > metadata. > > Add new initialisation API that will store the desired uid/gid and a > wrapper for the current use. Additionally add docs for the two APIs. > --- > src/storage/storage_backend.h | 3 +++ > src/storage/storage_driver.c | 39 ++++++++++++++++++++++++++++++++++++++- > src/storage/storage_driver.h | 5 +++-- > 3 files changed, 44 insertions(+), 3 deletions(-) > > +/** > + * virStorageFileInitAs: > + * > + * @src: storage source definition > + * @uid: uid to access the file as, -1 for current uid > + * @gid: gid to access the file as, -1 for current gid Correct grammar as written, but didn't flow well and took me two reads to avoid confusion. Would be easier with the addition of "or", as in: @xid: xid to access the file as, or -1 for current xid or even: @xid: xid used to access the file, or -1 for current xid > > + if (uid == (uid_t) -1) > + src->drv->uid = geteuid(); > + else > + src->drv->uid = uid; Do we need to do the conversion here, or can we store -1 and let other routines later do the conversion? I'm not sure if it matters either way, so I'm fine leaving it this way.

> +int > +virStorageFileInit(virStorageSourcePtr src) > +{ > + return virStorageFileInitAs(src, (uid_t) -1, (gid_t) -1); Casts aren't strictly necessary on this line (the C compiler does the correct conversion from int to uid_t thanks to the function prototype).

ACK with the comment and cast cleanup.