Is jdm_delete_filehandle part of a public API?

[Eric Sandeen]

I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.

Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...

anyone know?

If needed I guess I can just call it with hlen==0, but that seems odd.

Thanks,
-Eric

[Mark Tinguely]

Post by Eric Sandeen
I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.
Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...
anyone know?
If needed I guess I can just call it with hlen==0, but that seems odd.
Thanks,
-Eric

The first thing that comes to mind is maybe they trying to distinguish
between a fshandle or handle. Or they we trying to be consistent with
the allocation calls.

The libhandle free_handle has the same calling parameters. It also does
nothing with the length. That we cannot change without breaking existing
code.

I will look/ask around.

--Mark.

[Mark Tinguely]

Post by Mark Tinguely

Post by Eric Sandeen
I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.
Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...
anyone know?
If needed I guess I can just call it with hlen==0, but that seems odd.
Thanks,
-Eric

The first thing that comes to mind is maybe they trying to distinguish
between a fshandle or handle. Or they we trying to be consistent with
the allocation calls.
The libhandle free_handle has the same calling parameters. It also does
nothing with the length. That we cannot change without breaking existing
code.
I will look/ask around.
--Mark.

Looks like the code is pretty sloppy with freeing the handles.

Best guess is jdm_delete_filehandle() and free_handle are trying to
keep the API similar to DMAPI. The DMAPI handle free routine,
dm_handle_free(), also has a second length parameter that is not used in
the library.

The code example that I saw are similar to the use in xfsdump, where the
length used in the free comes from the handle creation/conversion routine.

Since the xfsprogs do not open handles with calls that provide a length.
How about FSHANDLE_SZ and FILEHANDLE_SZ depending on if it is a
xfs_fshandle or xfs_handle?

--Mark.

[Eric Sandeen]

Post by Mark Tinguely

Post by Mark Tinguely

Post by Eric Sandeen
I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.
Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...
anyone know?
If needed I guess I can just call it with hlen==0, but that seems odd.
Thanks,
-Eric

The first thing that comes to mind is maybe they trying to distinguish
between a fshandle or handle. Or they we trying to be consistent with
the allocation calls.
The libhandle free_handle has the same calling parameters. It also does
nothing with the length. That we cannot change without breaking existing
code.
I will look/ask around.
--Mark.

Looks like the code is pretty sloppy with freeing the handles.

yeah, that's what I was going to fix :)

Post by Mark Tinguely
Best guess is jdm_delete_filehandle() and free_handle are trying to
keep the API similar to DMAPI. The DMAPI handle free routine,
dm_handle_free(), also has a second length parameter that is not used
in the library.
The code example that I saw are similar to the use in xfsdump, where
the length used in the free comes from the handle creation/conversion
routine.

yup but I don't think jdm_getfshandle has anything similar does it?

Post by Mark Tinguely
Since the xfsprogs do not open handles with calls that provide a
length. How about FSHANDLE_SZ and FILEHANDLE_SZ depending on if it is
a xfs_fshandle or xfs_handle?

*shrug* it's not used anyway - I'm not sure why we'd need to invent a
macro to pass in only to have it ignored. Is there any advantage to that?


-Eric

Post by Mark Tinguely
--Mark.
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Mark Tinguely]

Post by Eric Sandeen

Post by Mark Tinguely

Post by Mark Tinguely

Post by Eric Sandeen
I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.
Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...
anyone know?
If needed I guess I can just call it with hlen==0, but that seems odd.
Thanks,
-Eric

The first thing that comes to mind is maybe they trying to distinguish
between a fshandle or handle. Or they we trying to be consistent with
the allocation calls.
The libhandle free_handle has the same calling parameters. It also does
nothing with the length. That we cannot change without breaking existing
code.
I will look/ask around.
--Mark.

Looks like the code is pretty sloppy with freeing the handles.

yeah, that's what I was going to fix :)

Post by Mark Tinguely
Best guess is jdm_delete_filehandle() and free_handle are trying to
keep the API similar to DMAPI. The DMAPI handle free routine,
dm_handle_free(), also has a second length parameter that is not used
in the library.
The code example that I saw are similar to the use in xfsdump, where
the length used in the free comes from the handle creation/conversion
routine.

yup but I don't think jdm_getfshandle has anything similar does it?

nope. Do you know why there is a jdm and a libhandle libs?

Post by Eric Sandeen

Post by Mark Tinguely
Since the xfsprogs do not open handles with calls that provide a
length. How about FSHANDLE_SZ and FILEHANDLE_SZ depending on if it is
a xfs_fshandle or xfs_handle?

*shrug* it's not used anyway - I'm not sure why we'd need to invent a
macro to pass in only to have it ignored. Is there any advantage to that?

never mind... handles are opaque and we should not be defining a size.

I did the grep and saw that the sizes were defined and thought they were
better than nothing. I did not not realize that the defines are are in
jdm.c and not a header file. In that case, nothing is better than adding
a define for an opaque item.

Post by Eric Sandeen
-Eric

--Mark.

[Dave Chinner]

Post by Eric Sandeen
I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.
Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...

Which means it is part of the published API, regardless of whether
it is documented in the man page or not.

As it is, we've never really cared about ABI stability for these
libraries - I don't think we've ever guaranteed stability in any
way. If you want to change it, though, I suspect we'll need to bump
the library version....

Cheers,

Dave.

[Christoph Hellwig]

Post by Eric Sandeen
I was cleaning up xfsprogs to plug some leaks, and wanted to use
jdm_delete_filehandle(). I noticed that it has an "hlen" argument which
is unused.
Can we remove that, or is this part of a public API? It's not in any
manpage (or even called anywhere in xfsprogs/xfstests/xfsdump/dmapi)
but it is in a public header...
anyone know?
If needed I guess I can just call it with hlen==0, but that seems odd.

If it's in a public header we shouldn't touch it unless there's a
pressing need.


Talking about libhandle: this one has been a bit bitrotted. Maybe it's
a good time to move everything over to the kernel by handle syscalls
and deprecated it?

[Dave Chinner]

Post by Christoph Hellwig
Talking about libhandle: this one has been a bit bitrotted. Maybe it's
a good time to move everything over to the kernel by handle syscalls
and deprecated it?

First you need to make the kernel by-handle interfaces handle the
sae functionality as the XFS by-handle ioctls.

1. O_NOCMTIME doesn't exist, and so there's no way to do
invisible IO on files.
2. Can we construct VFs filehandles in userspace from
bulkstat information (dump, xfs_fsr and others rely on
this capability)?
3. The kernel by-handle interfaces cannot manipulate
attributes at all (i.e. attr-list, attr-multi
functionality).

So until the VFS by-handle interfaces can do these things, we can't
change libhandle over to use the newer kernel interfaces.

I'm also pretty sure the incompatible handle format would mean
big problems if someone were to be storing file handles in userspace
and they upgrade their version of libhandle. i.e. such a library
implementation change is not forwards or backwards compatible with
existing applications, but bumping the major shared library version
should solve that problem.

However, these are all solvable problems. It's definitely a low
priority for me to do this, but if you want to do it patches will
definitely be accepted ;)

Cheers,

Dave.