On May 3, 2013, at 10:41 AM, Jeremy Allison <jra@xxxxxxxxx> wrote:
>> +glfd_fd_store (glfs_fd_t *glfd)
>> +{
>> + int i;
>> + void *tmp;
>> +
>> + if (glfd_fd_size == glfd_fd_used) {
>> + tmp = realloc (glfd_fd, (glfd_fd_size + 1024) * sizeof (glfs_fd_t *));
>
> Can you use talloc functions instead of malloc/realloc/free ?
> Yes, they are functionally identical (except talloc is a little
> slower :-) but even tallocing off the NULL context allows us
> to keep track of talloced memory and query it remotely, which
> can aid in debugging if there's anything wrong.
OK.
>> + if (!tmp) {
>> + errno = ENOMEM;
>> + return -1;
>> + }
>> +
>> + glfd_fd = tmp;
>> + memset(glfd_fd+glfd_fd_size, 0, 1024 * sizeof(glfs_fd_t *));
>> + glfd_fd_size += 1024;
>
> A minor nit here. Doing glfd_fd_size += 1024 and then
> using it in the realloc later doesn't protect against
> integer wrap. Yes I know this is an internal variable,
> not accessible to external users, but it's bad programming
> practice to not check for integer wrap on anything that's
> used for allocation - just as a matter of principle.
>
> (Yeah we probably do it ourselves elsewhere in the code,
> but it's still a bad thing and I fix it to add wrap tests
> whenever I come across it :-).
OK.
>> +smb_stat_ex_from_stat (struct stat_ex *dst, const struct stat *src)
>> +{
>> + ZERO_STRUCTP(dst);
>> +
>> + dst->st_ex_dev = 42;
>> + dst->st_ex_ino = src->st_ino;
>
> Is hard-coding st_ex_dev to 42 a good idea ?
> Can you not use a hash of the external volume
> name or something similar (I must confess I'm
> haven't completely studied how gluster does
> this).
>
> Is it possible to have 2 Samba shares which
> point at 2 separate gluster shared storage
> instances (which are completely separate
> backend systems) ? If so then you need to
> have a distinct st_ex_dev for each instance.
> dev/ino pairs are used as hash keys for tdbs.
>
> Other than these things looks good to me !
This is a debugging relic left over when I was getting spurious "dev/ino" mismatch in smbd.log. Will remove it in next submission.
Is it important for the st_ex_dev to be "stable" across reboots and across various smbds in clustered samba (for ctdb)?
Thanks for the review
Avati
