Make libcephfs return error when unmounted?

Make libcephfs return error when unmounted?

[Noah Watkins]

Hi all,

A simple program like this:

int main(int argc, char **argv)
{
        int ret;
        struct ceph_mount_info *cmount;

        ceph_create(&cmount, NULL);
        //ceph_mount(cmount, NULL);
        ceph_chdir(cmount, "/");
}

will segfault because in the below snippet, cmount->get_client() returns NULL when ceph_mount(..) has not been been called with success.

extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s)                                                                             
{                                                                                                                                                     
        return cmount->get_client()->chdir(s);                                                                                                             
}

It would be very useful to get a uniform error return value rather than the fault. Something like this came to mind:

diff --git a/src/libcephfs.cc b/src/libcephfs.cc
index b1481e6..4751e8f 100644
--- a/src/libcephfs.cc
+++ b/src/libcephfs.cc
@@ -180,6 +180,10 @@ public:
     return cct;
   }
 
+  bool is_mounted(void) {
+    return mounted;
+  }
+
 private:
   uint64_t msgr_nonce;
   bool mounted;
@@ -282,6 +286,8 @@ extern "C" const char* ceph_getcwd(struct ceph_mount_info *cmount)
 
 extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s)
 {
+  if (!cmount->is_mounted())
+    return -1004;
   return cmount->get_client()->chdir(s);
 }

Any thoughts on a good way to handle this?

-Noah

Re: Make libcephfs return error when unmounted?

[Greg Farnum]

On Wednesday, April 11, 2012 at 11:12 AM, Noah Watkins wrote:
> Hi all,
> 
> A simple program like this:
> 
> int main(int argc, char **argv)
> {
> int ret;
> struct ceph_mount_info *cmount;
> 
> ceph_create(&cmount, NULL);
> //ceph_mount(cmount, NULL);
> ceph_chdir(cmount, "/");
> }
> 
> will segfault because in the below snippet, cmount->get_client() returns NULL when ceph_mount(..) has not been been called with success.
> 
> extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s) 
> { 
> return cmount->get_client()->chdir(s); 
> }
> 
> It would be very useful to get a uniform error return value rather than the fault. Something like this came to mind:
> 
> diff --git a/src/libcephfs.cc (http://libcephfs.cc) b/src/libcephfs.cc (http://libcephfs.cc)
> index b1481e6..4751e8f 100644
> --- a/src/libcephfs.cc (http://libcephfs.cc)
> +++ b/src/libcephfs.cc (http://libcephfs.cc)
> @@ -180,6 +180,10 @@ public:
> return cct;
> }
> 
> + bool is_mounted(void) {
> + return mounted;
> + }
> +
> private:
> uint64_t msgr_nonce;
> bool mounted;
> @@ -282,6 +286,8 @@ extern "C" const char* ceph_getcwd(struct ceph_mount_info *cmount)
> 
> extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s)
> {
> + if (!cmount->is_mounted())
> + return -1004;
> return cmount->get_client()->chdir(s);
> }
> 
> Any thoughts on a good way to handle this?
> 
> -Noah 
I'm not sure where the "-1004" came from, but I've been doing things very much like this in my code recently. Functions that require simple preconditions (well, really any preconditions) like that should document them, have defined behavior if they're not met, and return defined error codes. I believe our new code does, but <insert legacy code excuse>. 

Patches welcome, of course, but if it's still a problem this summer we can probably put an intern on it for a day to help them understand why standard programming practices are good things. ;)

-Greg

Re: Make libcephfs return error when unmounted?

[Yehuda Sadeh Weinraub]

On Wed, Apr 11, 2012 at 11:12 AM, Noah Watkins <jayhawk@cs.ucsc.edu> wrote:
>
> Hi all,
>
> A simple program like this:
>
> int main(int argc, char **argv)
> {
>        int ret;
>        struct ceph_mount_info *cmount;
>
>        ceph_create(&cmount, NULL);
>        //ceph_mount(cmount, NULL);
>        ceph_chdir(cmount, "/");
> }
>
> will segfault because in the below snippet, cmount->get_client() returns
> NULL when ceph_mount(..) has not been been called with success.
>
> extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s)
> {
>        return cmount->get_client()->chdir(s);
> }
>
> It would be very useful to get a uniform error return value rather than
> the fault. Something like this came to mind:
>
> diff --git a/src/libcephfs.cc b/src/libcephfs.cc
> index b1481e6..4751e8f 100644
> --- a/src/libcephfs.cc
> +++ b/src/libcephfs.cc
> @@ -180,6 +180,10 @@ public:
>     return cct;
>   }
>
> +  bool is_mounted(void) {
> +    return mounted;
> +  }
> +
>  private:
>   uint64_t msgr_nonce;
>   bool mounted;
> @@ -282,6 +286,8 @@ extern "C" const char* ceph_getcwd(struct
> ceph_mount_info *cmount)
>
>  extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s)
>  {
> +  if (!cmount->is_mounted())
> +    return -1004;
>   return cmount->get_client()->chdir(s);
>  }
>
> Any thoughts on a good way to handle this?

Also need to check that cmount is initialized.  I'd add a helper:

Client *ceph_get_client(struct ceph_mount_info *cmont)
{
  if (cmount && cmount->is_mounted())
    return cmount->get_client();

  return NULL;
}

extern "C" int ceph_chdir (struct ceph_mount_info *cmount, const char *s)
{
  Client *client = ceph_get_client(cmount);
  if (!client)
    return -EINVAL;

  return client->chdir(s);
}


>
> -Noah--
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Make libcephfs return error when unmounted?

[Noah Watkins]

On Apr 11, 2012, at 11:22 AM, Greg Farnum wrote:

> On Wednesday, April 11, 2012 at 11:12 AM, Noah Watkins wrote:
>> Hi all,
>> 
>> -Noah 
> I'm not sure where the "-1004" came from,

ceph_mount(..) seems to return some random error codes (-1000, 1001) already  :)

> Patches welcome, of course, but if it's still a problem this summer we can probably put an intern on it for a day to help them understand why standard programming practices are good things. ;)

I'll happily create a patch for this :) Any method to come up with Ceph-specific error codes?

-Noah

Re: Make libcephfs return error when unmounted?

[Greg Farnum]

On Wednesday, April 11, 2012 at 11:25 AM, Noah Watkins wrote:
> 
> On Apr 11, 2012, at 11:22 AM, Greg Farnum wrote:
> 
> > On Wednesday, April 11, 2012 at 11:12 AM, Noah Watkins wrote:
> > > Hi all,
> > > 
> > > -Noah 
> > I'm not sure where the "-1004" came from,
> 
> ceph_mount(..) seems to return some random error codes (-1000, 1001) already :)

<grumble> legacy undocumented grr </grumble>
Let's try to use standard error codes where available, and (if we have to create our own) document any new ones with user-accessible names and explanations. I don't know which one is "best" but I see a lot of applicable choices when scanning errno-base et al.

Also, what Yehuda said. :)
-Greg

Re: Make libcephfs return error when unmounted?

[Noah Watkins]

On Apr 11, 2012, at 11:22 AM, Yehuda Sadeh Weinraub wrote:

> Also need to check that cmount is initialized.  I'd add a helper:
> 
> Client *ceph_get_client(struct ceph_mount_info *cmont)
> {
>  if (cmount && cmount->is_mounted())
>    return cmount->get_client();
> 
>  return NULL;
> }

How useful is checking cmount != NULL here? This defensive check depends on users initializing their cmount pointers to NULL, but the API doesn't do anything to require this initialization assumption.

- Noah

Re: Make libcephfs return error when unmounted?

[Greg Farnum]

On Wednesday, April 11, 2012 at 2:59 PM, Noah Watkins wrote:
> 
> On Apr 11, 2012, at 11:22 AM, Yehuda Sadeh Weinraub wrote:
> 
> > Also need to check that cmount is initialized. I'd add a helper:
> > 
> > Client *ceph_get_client(struct ceph_mount_info *cmont)
> > {
> > if (cmount && cmount->is_mounted())
> > return cmount->get_client();
> > 
> > return NULL;
> > }
> 
> 
> 
> How useful is checking cmount != NULL here? This defensive check depends on users initializing their cmount pointers to NULL, but the API doesn't do anything to require this initialization assumption.
> 
> - Noah 
I had a whole email going until I realized you were just right. So, yeah, that wouldn't do anything since a cmount they forgot to have the API initialize is just going to hold random data. Urgh.
-Greg

Re: Make libcephfs return error when unmounted?

[Noah Watkins]

On Apr 11, 2012, at 3:13 PM, Greg Farnum wrote:

> On Wednesday, April 11, 2012 at 2:59 PM, Noah Watkins wrote:
>> 
>> On Apr 11, 2012, at 11:22 AM, Yehuda Sadeh Weinraub wrote:
>> 
>>> Also need to check that cmount is initialized. I'd add a helper:
>>> 
>>> Client *ceph_get_client(struct ceph_mount_info *cmont)
>>> {
>>> if (cmount && cmount->is_mounted())
>>> return cmount->get_client();
>>> 
>>> return NULL;
>>> }
>> 
>> 
>> 
>> How useful is checking cmount != NULL here? This defensive check depends on users initializing their cmount pointers to NULL, but the API doesn't do anything to require this initialization assumption.
>> 
>> - Noah 
> I had a whole email going until I realized you were just right. So, yeah, that wouldn't do anything since a cmount they forgot to have the API initialize is just going to hold random data. Urgh.

One could pair the pointer with a magic value in a separate structure, but even libc doesn't go to these lengths to protect users...

Re: Make libcephfs return error when unmounted?

[Noah Watkins]

On Apr 11, 2012, at 11:32 AM, Greg Farnum wrote:

> On Wednesday, April 11, 2012 at 11:25 AM, Noah Watkins wrote:
>> 
>> On Apr 11, 2012, at 11:22 AM, Greg Farnum wrote:
>> 
>>> On Wednesday, April 11, 2012 at 11:12 AM, Noah Watkins wrote:
>>>> Hi all,
>>>> 
>>>> -Noah 
>>> I'm not sure where the "-1004" came from,
>> 
>> ceph_mount(..) seems to return some random error codes (-1000, 1001) already :)
> 
> <grumble> legacy undocumented grr </grumble>
> Let's try to use standard error codes where available, and (if we have to create our own) document any new ones with user-accessible names and explanations. I don't know which one is "best" but I see a lot of applicable choices when scanning errno-base et al.

If I'm choosing from from errno-base I might go with

  #define ENXIO        6  /* No such device or address */

It's used:

osd/OSD.cc
void OSD::handle_misdirected_op(PG *pg, OpRequest *op)
…
reply_op_error(op, -ENXIO)

I'm wondering if you happen to know if this will be propagated back to the client? I'd be nice to have an exclusive not-mounted condition on the client side.

-Noah--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Make libcephfs return error when unmounted?

[Yehuda Sadeh Weinraub]

On Wed, Apr 11, 2012 at 3:13 PM, Greg Farnum
<gregory.farnum@dreamhost.com> wrote:
> On Wednesday, April 11, 2012 at 2:59 PM, Noah Watkins wrote:
>>
>> On Apr 11, 2012, at 11:22 AM, Yehuda Sadeh Weinraub wrote:
>>
>> > Also need to check that cmount is initialized. I'd add a helper:
>> >
>> > Client *ceph_get_client(struct ceph_mount_info *cmont)
>> > {
>> > if (cmount && cmount->is_mounted())
>> > return cmount->get_client();
>> >
>> > return NULL;
>> > }
>>
>>
>>
>> How useful is checking cmount != NULL here? This defensive check depends on users initializing their cmount pointers to NULL, but the API doesn't do anything to require this initialization assumption.
>>
>> - Noah
> I had a whole email going until I realized you were just right. So, yeah, that wouldn't do anything since a cmount they forgot to have the API initialize is just going to hold random data. Urgh.

There's no destructor either, maybe it's a good time to add one?

Yehuda

Re: Make libcephfs return error when unmounted?

[Greg Farnum]

On Wednesday, April 11, 2012 at 3:18 PM, Noah Watkins wrote:
> One could pair the pointer with a magic value in a separate structure, but even libc doesn't go to these lengths to protect users...

At that point we'd run a pretty high risk of segfaulting when trying to deref and look at the magic value anyway. Nothing useful we can do here, I think. :/  




On Wednesday, April 11, 2012 at 3:29 PM, Noah Watkins wrote:

>  
> On Apr 11, 2012, at 11:32 AM, Greg Farnum wrote:
>  
> > On Wednesday, April 11, 2012 at 11:25 AM, Noah Watkins wrote:
> > >  
> > > On Apr 11, 2012, at 11:22 AM, Greg Farnum wrote:
> > >  
> > > > On Wednesday, April 11, 2012 at 11:12 AM, Noah Watkins wrote:
> > > > > Hi all,
> > > > >  
> > > > > -Noah  
> > > > I'm not sure where the "-1004" came from,
> > >  
> > >  
> > >  
> > > ceph_mount(..) seems to return some random error codes (-1000, 1001) already :)
> >  
> > <grumble> legacy undocumented grr </grumble>
> > Let's try to use standard error codes where available, and (if we have to create our own) document any new ones with user-accessible names and explanations. I don't know which one is "best" but I see a lot of applicable choices when scanning errno-base et al.
>  
>  
>  
> If I'm choosing from from errno-base I might go with
>  
> #define ENXIO 6 /* No such device or address */
>  
> It's used:
>  
> osd/OSD.cc (http://OSD.cc)
> void OSD::handle_misdirected_op(PG *pg, OpRequest *op)
> …
> reply_op_error(op, -ENXIO)
>  
> I'm wondering if you happen to know if this will be propagated back to the client? I'd be nice to have an exclusive not-mounted condition on the client side.
>  
> -Noah  
Unfortunately it is. (I just grepped and see it's returned by both the MDS and the OSD, but it's not in Objecter et al so it's not filtered out anywhere.)

ENODEV isn't returned from anything; do you think that makes sense?
-Greg



--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Make libcephfs return error when unmounted?

[Greg Farnum]

On Wednesday, April 11, 2012 at 3:34 PM, Yehuda Sadeh Weinraub wrote:

> On Wed, Apr 11, 2012 at 3:13 PM, Greg Farnum
> <gregory.farnum@dreamhost.com (mailto:gregory.farnum@dreamhost.com)> wrote:
> > On Wednesday, April 11, 2012 at 2:59 PM, Noah Watkins wrote:
> > > 
> > > On Apr 11, 2012, at 11:22 AM, Yehuda Sadeh Weinraub wrote:
> > > 
> > > > Also need to check that cmount is initialized. I'd add a helper:
> > > > 
> > > > Client *ceph_get_client(struct ceph_mount_info *cmont)
> > > > {
> > > > if (cmount && cmount->is_mounted())
> > > > return cmount->get_client();
> > > > 
> > > > return NULL;
> > > > }
> > > 
> > > 
> > > 
> > > 
> > > 
> > > How useful is checking cmount != NULL here? This defensive check depends on users initializing their cmount pointers to NULL, but the API doesn't do anything to require this initialization assumption.
> > > 
> > > - Noah
> > I had a whole email going until I realized you were just right. So, yeah, that wouldn't do anything since a cmount they forgot to have the API initialize is just going to hold random data. Urgh.
> 
> 
> 
> There's no destructor either, maybe it's a good time to add one?
> 
> Yehuda 
Actually, there is. The problem is that to the client it's an opaque pointer under many(most?) circumstances, so that it can be used by C users. 
-Greg

Re: Make libcephfs return error when unmounted?

[Noah Watkins]

On Apr 11, 2012, at 3:39 PM, Greg Farnum wrote:

> On Wednesday, April 11, 2012 at 3:18 PM, Noah Watkins wrote:
>> One could pair the pointer with a magic value in a separate structure, but even libc doesn't go to these lengths to protect users...
> 
> At that point we'd run a pretty high risk of segfaulting when trying to deref and look at the magic value anyway. Nothing useful we can do here, I think. :/

Actually I meant:

struct container {
  int magic;
  struct ceph_mount_info *ptr;
};

ceph_chdir(struct container something);

Then, container.magic should equal INITIALIZED where INITIALIZED is "some value that is unlikely to ever be present at a memory location where container is allocated". Lol… overkill and not bullet-proof.

> On Wednesday, April 11, 2012 at 3:29 PM, Noah Watkins wrote:
> 
>> 
>> On Apr 11, 2012, at 11:32 AM, Greg Farnum wrote:
>> 
>>> On Wednesday, April 11, 2012 at 11:25 AM, Noah Watkins wrote:
>>>> 
>>>> On Apr 11, 2012, at 11:22 AM, Greg Farnum wrote:
>>>> 
>>>>> On Wednesday, April 11, 2012 at 11:12 AM, Noah Watkins wrote:
>>>>>> Hi all,
>>>>>> 
>>>>>> -Noah  
>>>>> I'm not sure where the "-1004" came from,
>>>> 
>>>> 
>>>> 
>>>> ceph_mount(..) seems to return some random error codes (-1000, 1001) already :)
>>> 
>>> <grumble> legacy undocumented grr </grumble>
>>> Let's try to use standard error codes where available, and (if we have to create our own) document any new ones with user-accessible names and explanations. I don't know which one is "best" but I see a lot of applicable choices when scanning errno-base et al.
>> 
>> 
>> 
>> If I'm choosing from from errno-base I might go with
>> 
>> #define ENXIO 6 /* No such device or address */
>> 
>> It's used:
>> 
>> osd/OSD.cc (http://OSD.cc)
>> void OSD::handle_misdirected_op(PG *pg, OpRequest *op)
>> …
>> reply_op_error(op, -ENXIO)
>> 
>> I'm wondering if you happen to know if this will be propagated back to the client? I'd be nice to have an exclusive not-mounted condition on the client side.
>> 
>> -Noah  
> Unfortunately it is. (I just grepped and see it's returned by both the MDS and the OSD, but it's not in Objecter et al so it's not filtered out anywhere.)
> 
> ENODEV isn't returned from anything; do you think that makes sense?
> -Greg
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html