Skip to contentSkip to conversation

Is jdm_delete_filehandle part of a public API?

DISCUSSION Eric Sandeen 5 participants 8 messages Jul 29, 2014 – Aug 4, 2014

lore
Share & tools
lore
#0 lore raw

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

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw
Quoted 8 lines · 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...

Quoted 6 lines

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.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw
Quoted 20 lines · Mark Tinguely
Quoted 8 lines · 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...

Quoted 6 lines

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.

Quoted 7 lines

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.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#3 lore raw
Quoted 25 lines · Mark Tinguely
Quoted 24 lines · Mark Tinguely
Quoted 8 lines · 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...

Quoted 6 lines

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.

Quoted 5 lines
Quoted 3 lines

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 :)

Quoted 8 lines

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?

Quoted 3 lines

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

Quoted 8 lines

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw
Quoted 39 lines · Eric Sandeen
Quoted 25 lines · Mark Tinguely
Quoted 24 lines · Mark Tinguely
Quoted 8 lines · 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...

Quoted 6 lines

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.

Quoted 5 lines
Quoted 3 lines

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 :)

Quoted 5 lines

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.

Quoted 5 lines
Quoted 3 lines

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?

Quoted 7 lines
Quoted 4 lines

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.

Quoted 1 line

-Eric

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw
In reply to Eric Sandeen
Quoted 7 lines · 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.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#6 lore raw
In reply to Eric Sandeen
Quoted 11 lines · 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?

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#7 lore raw
Quoted 3 lines · 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.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs